"Pavol Droba" <droba@topmail.sk> wrote in message news:1256163046.20040926162826@topmail.sk...
Hi all,
The review of the Smart Container library, written by Thorsten Ottosen starts today (September 26th, 2004) and runs for 10 days (until October 5th, 2004).
Hi, 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.
* 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 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. 3. I think Cloner sounds better than CloneManager. Cloner is already an English word; CloneManager sound like a mixed metaphor. 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. 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. 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] >. 7. I don't understand review issue 10. I don't see the erase()/insert() functions that return pair<iterator, bool>. 8. For possible review issue 1, I think it's no big deal that you can't use std::stack to adapt a ptr_container. 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. 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.
* 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: 1. Clonable is first said to refine Copy Constructible, but then the docs state "a type T might be Clonable even though it is not Assignable or Copy Constructible " 2. Under the heading "Clone Manager", it looks like 1. and 2. might be reversed; at any rate it's not clear. 3. The Clone Manager notation table should be put in the canonical form, e.g., CloneManger - A type modeling Clone Manager cm, cm2 - Objects of type CloneManager 4. I'd prefer the acknoweldgements to say something like this: Jonathan Turkanis for supplying his move_ptr framework -- used internally by the library -- based on the work of Rani Sharoni, Daniel Wallin, Howard Hinnant and Bronek Kozicki. 5. In first line of indirect_predicate.html, 'what' should be 'want'. 6. On mozilla, some of the preformatted text overflows the bounding box, e.g., in "3. Copy-semantics of smart containers" 7. Under clonable concept, you say " If you are implementing a class inline in headers, remember to forward declare the functions ". You should elaborate on this. 8. In the first line after the heading "Terminology", the phrase 'an heap-allocated object' should probably be 'a heap allocated object'. 'an heap' sounds schoolmarmish. 9. The first example is way too long and is not a good introduction to the library. In fact, it's the reason I delayed writing a review until the last minute. A tutorial section should start by explaining the problem to be solved, and then show step by step how to solve it. One long block of code, even commented code, is too much to digest when one knows nothing about the library. 10. British and American cows say "Moo". 11. Sound that pigs make, according to google: oink - 160,000 hits oiink - 626 hits oiiink - 85 hits oiiiink - 15 hits oiiiiink - 18 hits oiiiiiink - 4 hits oiiiiiiink - 4 hits oiiiiiiiink - 1 hit oiiiiiiiiink - 5 hits oiiiiiiiiiink - 2 hits oiiiiiiiiiiink - 3 hits oiiiiiiiiiiiink - 3 hits oiiiiiiiiiiiiink - 1 hit oiiiiiiiiiiiiiink - 3 hits oiiiiiiiiiiiiiiink - 0 hits oiiiiiiiiiiiiiiiink - 36 hits oiiiiiiiiiiiiiiiiink - 1 hits
* What is your evaluation of the potential usefulness of the library?
Extremely useful.
* Did you try to use the library? With what compiler? Did you have any
problems? No.
* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study? About three hours.
* Are you knowledgeable about the problem domain?
Yes. Best Regards, Jonathan