Hi Jonathan, Thanks for your review. | Here's my review. I believe the Smart Container library should be ACCEPTED. I | have some suggested modifications, but there is only one issue which I think | needs to be resolved before I can give the library my unconditional approval | (item 5 under "evaluation of the design"). Since I did not have time to study | the implementation, my yes vote depends on other reviewers having done so and | found it acceptable. Thanks...am sure we can solve those issues. | > * What is your evaluation of the design? | | Very good, on the whole. I have these comments (in no particular order): | | 1. The functions used in the definition of Clonable are not well named. I would | prefer either | - allocate_clone/deallocate_clone or construct_clone/destroy_clone, to mimic | the interface of standard allocators, or | - something using new/delete, as someone else suggested yeah, there is some inconsistency here. | 2. It's a bit odd that CloneManager uses operator() to destroy objects. I | understand that this is a requirement of using CloneManager as a deleter, but | you could easily use an adapter instead. ok. | 3. I think Cloner sounds better than CloneManager. Cloner is already an English | word; CloneManager sound like a mixed metaphor. As I mentioned in another mail, and as sugeested by Pavol, it would probably be better to rip CloneManager apart into Allocator and CloneAllocator | 4. Since performance is one of the main design goals, I'm suprised that | attempting to insert a null pointer causes an exception rather than an insertion | failure. The same goes for invoking the functions for back, front, pop_back and | pop_front with an empty container. By coincidence, this morning I was | experimenting with some code that invokes a function pointer with a pointer as | argument, like so | | f_(p_) | | in an inner loop. I tried replacing the above code with | | if (p_) | f_(p_); | else | throw std::runtime_error(...); | | I found an enormous slowdown, which should be no surprise. yeah, I dunno, this might depend heavily on the compiler. what compiler do you use? At least for back()/front() you can avoid a check by *ptr_begin()/*--ptr_end(). For vector/deque you can unchecked random access too via operator[](). It is is true that performance was a design goal, but so was safety. The domain of pointers allowed me to do different trade-offs than with standard containers. For example, does it really matter if container.push_back( new T ) throw if 0 was inserted? IMO no, because the heap operation will dwarf any other operation by several magnitudes. The same considerations would be true for eg. pop_back() when calling the detructor on the object since we need to remove the object from the heap. | 5. The caveat "splice()/merge() in ptr_list is not thought through yet" makes me | very nervous. What aspect needs to be thought through? Does the current | implementation have well-defined semantics? I have not examined the | implementation, so I can't answer these questions, but whatever the caveat | means, resolving it should be a condition of acceptance. yes, I understand your concern and I promise to find a solution to this. It's just that each function needs to be faily carefully examined to see if its specs can potentially lead to leaks and other problems. I just didn't have the time, and I thought the review could work without people worriying too much about splice(). | 6. To answer possible review issue 13, I'd say you should parameterize the | provided clone managers by allocator type, defaulting to std::allocator< | [unspecified] >. why unspecified instead of T* ? | 7. I don't understand review issue 10. I don't see the erase()/insert() | functions that return pair<iterator, bool>. in set/map and ptr_set/ptr_map these functions returns this pair to say "we did/did not insert the element". I think that the same should hold for transfer; it would be only natural. Another way would of course to return optional<iterator>. | 9. For possible review issue 5, theoretcially I'd like to see auto_type exposed | as move_ptr. (I now like Howard Hinnant's suggested "unique_ptr", though he | seems to be leaning toward "sole_ptr" now.) Then you could also use move_ptr | where you now use auto_ptr. However, I agree with one of the other reviewers who | said that this would really be appropriate only if move_ptr were a stand-alone | library. I haven't proposed move_ptr mainly because I'm not sure whether it will | be made obsolete by policy_ptr and because while there was a great deal of | discussion in January, there wasn't much comment after I posted my recent | version. yeah, we'll wait and see. | 10. If ptr_iterators are kept, I agree that there needs to be a much more | detailed explanation of which algorithms they can be used with. ok. | > * What is your evaluation of the implementation? | | The parts I looked at looked good, but that means little since I only gave it a | quick glance. | | > * What is your evaluation of the documentation? | | It was hard to find my way around in the documentation. In general, there should | be a better index and more hyperlinks. My javascript menu is now a de facto part | of boost, so you might consider using it ;-) Here are some more detailed | comments, in no particular order: yes, good idea. [I have snipped most of your detailed comments, but I will look into them] | 6. On mozilla, some of the preformatted text overflows the bounding box, e.g., | in "3. Copy-semantics of smart containers" hm...yeah, I don't really know why it does that. | 10. British and American cows say "Moo". | | 11. Sound that pigs make, according to google: | | oink - 160,000 hits :-) | > * What is your evaluation of the potential usefulness of the library? | | Extremely useful. Thanks! br Thorsten