"John Torjo" <john.lists@torjo.com> wrote in message news:416178B9.5060702@torjo.com... | Thorsten Ottosen wrote: | | > 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? | | ptr_begin and ptr_end. isn't it easier just not to use them if you don't want? | > | > | 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. | | There are times when it's a good thing (the null object pattern). | But I'm really against creating such a class for each type I want to use | smart_container for. how often do we need the nulls? (ie, would you need it for all classes?) | Besides, there are operations which don't make sense for a "null object". can you give some examples? | And, for each new (virtual) function you add, you should update the | null_object class - which I think can turn into a maintenance nightmare :( if your inheritance hierarchy contains N distinct classes, you would need to add N implementations of that virtual function anyway, right? | > | ---------------------- | > | 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. | | I don't understand the above phrase. if you want to extend the std::map for some reason or make your own map, then you can do so and you can use that new map as a basis for a smart container too using ptr_map_adapter. | > | "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. | | But it seems to be move semantics ;) well, yes and no. "yes", stuff is moved, and "no" move semantics deal with rvalues by overloading copying for rvalue-references &&. It might sound wierd that x.move() does not require T to be moveable. But let's have Pavol or me make a vote about it. | > | ---------------------------- | > | 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. | | Yes, but for me - by reading CloneManager::operator(), I would think it | would provide a clone. Thus, it's misleading. | | I think it would me more straightforward not to use operator() in this | case. You could have a function, called 'destroy()' or something. I guess it might depend a bit on whether the auto_type should be exposed or not. I'd rather say move_ptr<T,CloneAllocator&> than move_ptr<T,SomeClass&> and then explaining that SomeClass calls CloneAllocator::destroy() from its operator()(). This would also require another object in the class + another indirection which might make it harder to optimize away if operator()() is empty. | > | 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? | | Like this: | std::map<key,value> m; | std::vector<key> k; | std::vector<value> v; | | rng::copy( transformed(pair_1st(m)), std::back_inserter(k) ); | rng::copy( transformed(pair_2nd(m)), std::back_inserter(v) ); nice :-) but what about rng::copy( map_keys( m ), std::back_inserter(k) ); rng.:copy( map_values( m ), std::back_inserter(k) ); so we don't have to go into the pair terminology? | > | ------------------------- | > | 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? | | Would you please exemplify? Thanks. for example, consider boost::ptr_set<T> set; boost::ptr_vector<T> vec; ... // fill vec with many duplicates as defined by operator< on T for( 0..vec.size() - 1 ) { // #1 set.insert( vec[i] ); // #2 set.insert( allocate_clone( vec[i] ) ); } #1 can check if vec[i] already exists as defined by operator< on T. If it does exists, there is no need to make a new clone. #2 always make a new clone and might need to delete it afterwards