* What is your evaluation of the design?
The design is ok. I'm still wondering if we really need two pairs of iterators (begin()/end() and ptr_begin()/ptr_end()). Also, I don't like the fact that you can't have null pointers but I can live with that ;)
* What is your evaluation of the implementation?
A quite 10-15 minutes glance. The implementation is quite ok. Side-note: maybe you can hide 'template.hpp' somewhere ;)
* What is your evaluation of the documentation?
The overall aspect is "good towards very good". A "few" notes though: The example at "Basic Usage" It is too big. You should focus on only what you're trying to show (smart_container). Also, I see no need for the notation: result function() { return blabla; } when it could be : result function() { return blabla; } Or, for bigger functions that are unimportant, you could simply say: result function() { /* ... */ } --------------------- Changing the Clone Manager You should first explain some of the internals, before jumping to "Clone Managers" and such. ---------------------- Extending a map I don't understand the need for my_map class which just derives from std::map. Why not use std::map directly? Relative to this: // // add constructors and assignment operators here, none needs // to be defined // What do you mean - they don't need to be defined? If I add a constructor or so, I should define it somewhere. Otherwise, what's the point of declaring it? Also, I assume the #include <string> and #include "foo.h" should come after the "// Usage" comment, otherwise they are misleading. ------------------------- Clone manager requirements "the Clone Manager need not be stateless, but the assumption is that it probably will be." Which is it? Can it be stateless or not? ------------------------- Reference: "Stored elements are required to be Clonable for a subset of the operations" By looking at the example, that makes me wonder how will you handle the same example for a view_clone_manager... "Ownership can be transferred from a container to another container on a per iterator range basis" I wonder if a bettern name for the transfer function would be "move()". ---------------------------- Class reversible_smart_container "All objects must be deallocable with CloneManager::operator()" This seems a very "smart" way of deallocating objects. Is this a typo? POSSIBLE REVIEW ISSUES 6. #Should iterator range versions of assign() and insert be replaced by an Boost.Range version? Ie, should we have template< class SinglePassRange > void insert( const Range& r ); instead of template< class InputIterator > void insert( InputIterator f, InputIterator l ); ?? Yes, I would prefer so. 9. #Should we add iterators for traversing the keys of a map? This would mean we could say map::key_iterator b = m.begin(), e = m.end(); copy( b, e, some_where ); I would not like that very much. As a side note, Boost Rangelib (formely know as Range Template Library - www.torjo.com/rangelib/) already provides means to walk through the keys or values of a collection. 12. #should we provide member algorithms for unsafe standard algorithms like unique() and remove? Why would you say "unsafe"? Maybe I'm overlooking something, but std::unique/std::remove should work fine now. ------------------------- FAQ Why are inserting member functions overloaded for both pointers and references? REVIEW QUESTION: do we want this behavior? No.
* What is your evaluation of the potential usefulness of the library?
Very 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?
A little more than 1 1/2 hours.
* Are you knowledgeable about the problem domain?
Yes.
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
The library should be ACCEPTED. Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)