[boost] Re: Re: [review] Formal review of the SmartContainer librarystarts today

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-10-04 11:34:51

"John Torjo" <john.lists_at_[hidden]> wrote in message
news:416178B9.5060702_at_torjo.com...
| Thorsten Ottosen wrote:
|
| > Hi John,
| >
| > Thanks for the  review.
| >
| > "John Torjo" <john.lists_at_[hidden]> wrote in message
| > news:415EF071.3040204_at_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