Hi John, Thanks for the review. "John Torjo" <john.lists@torjo.com> wrote in message news:415EF071.3040204@torjo.com... | | > | > * 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()). so which would you remove? | Also, I don't like the fact that you can't have null pointers but I can | live with that ;) ok, do you find the null object pattern undesirable? I actually think it is a strength. | | > * 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 ;) he he, yes it should be removed. | | > * 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). ok. noted. | 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() { /* ... */ } agreed. | | | --------------------- | Changing the Clone Manager | | You should first explain some of the internals, before jumping to "Clone | Managers" and such. ah yes, the tutorial can be much better...and will be eventually. | ---------------------- | 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? It's beyond what I want to do myself, but the idea should be that you can if you want. | 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? the point is that in this approach you don't need to define more than you want; the example above requires you to define standard map contructors because the ptr_map code depends on those constructors. | Also, I assume the #include <string> and #include "foo.h" should come | after the "// Usage" comment, otherwise they are misleading. ok. | ------------------------- | 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? it can be and the two default ones are. I need to express this clearer. | ------------------------- | 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... This is not necessay: -------------- // a class that has no normal copy semantics class X : boost::noncopyable { public: X* clone() const; ... }; // this will be found by the library by argument dependent lookup X* allocate_clone( const X& x ) { return x.clone(); } -------- this is the same: --------------- // we can now use the interface that requires clonability ptr_vector<X,view_clone_manager> vec1, vec2; ... vec2 = vec1.clone(); vec2.insert( vec2.end(), vec1.begin(), vec1.end() ); ------------- but with new semantics | "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()". Move sounds ok. It might be confused with move semantics though. | | | ---------------------------- | 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? not sure what you mean. operator()() is from the clone manager is used as a deleter all over the place, internally, that is were the requirement stems from. | | 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. ok. | 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. ok, how does it look like? | 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. depends on what iterator you use. when using pointers these algorithms can remove pointers and duplicate others in the containers. | ------------------------- | FAQ | | Why are inserting member functions overloaded for both pointers and | references? | REVIEW QUESTION: do we want this behavior? | | No. ok, also if it means loss of optimality in for map/set with uinque keys? | > * What is your evaluation of the potential usefulness of the library? | | Very useful. | > 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. Thanks! br Thorsten