Hi Thom, 2007/5/22, Thom Holwerda <slakje@xxxxxxxxxxx>:
Comments greatly appreciated!
The patch applies cleanly, so that's a good start. Some notes: - have to be copied into a new piece of allocated memory which, in turn, is an + have to be copied into a new piece of allocated memory, which, in turn, is an Are the extra commas around between 'in turn' necessary? It might be correct, but it looks a bit awkward (3 comma's in a row). Plus, in regular speech you would not pronounce it as which (pause) in turn (high pitch ending, pause), because it not realy is in turn anyway. I actually would like to remove the in turn at all since the allocation is expensive, and it's not hte expensiveness that is a result of the fact that you allocate new memory. (does that make sense?) - If you are unsure, you don't have to overthink this. Just make sure you don't - use a lot of lists and as long as the list isn't used in one of the + If you are unsure, you do not have to over-think this. Just make sure you do not use a lot of lists, and as long as the list is not used in one of the I guess overthinking is from my original version, and I'm affraid that it's a dutchism, since the dictionary does not mention you can think over something, let alone overthink something. Rather change it to 'not to worry too much'. /*! - \name Adding and removing items + \name Adding and removing items. */ I think both the original and the new version are wrong. This is a header, as such it should be 'Adding and Removing Items'. I bet the other headers share the same problem. - \param index The offset in the list where to put the item. + \param index The offset in the list where the item should be put. Though your improvement may be more correct, it sounds a bit awkward. Should we replace it with 'placed' or 'end up'? - \warning If there is anything you want, for which you need the list of - objects, please understand that that probably means that what you want to do - is a bad idea to begin with and you should avoid this method. The list of - objects doesn't belong to you. See if DoForEach() can help you out instead. + \warning If there is anything you want, for which you need the list of objects, please realize that that probably means that what you want to do is a bad idea to begin with and that you should avoid this method. The list of objects does not belong to you. See if DoForEach() can help you out instead. The wrapping did not go very well there. Your editor ate up all the newlines and now it's one big line. I think there is another case where I suddenly saw some lines disappear. Could you please doublecheck this again. If you fix the problem of the eaten newlines and think about my commentary (which you are free to accept or reject), I will accept the next patch, though probably tomorrow, since I'll be going home in a few minutes. Niels