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

From: John Torjo (john.lists_at_[hidden])
Date: 2004-10-05 02:59:51

> | > |
> | > | 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?

hmmm.. ok ;)

> 
> | >
> | > | 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?)

I can't make an estimate (which would only be subjective), but I think 
there are cases.

Another problem, IMO, is that client code will need to be more 
complicated, in order to find out if a value is null or not.

> 
> | Besides, there are operations which don't make sense for a "null object".
> 
> can you give some examples?

How about, cloning ;) ?
Of course, you can always make a null_object class with all functions 
throwing an error.

> 
> | 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?

Not necessary... I can add one implementation, and some of those 
distinct N classes don't have to override it ;)

> 
> | > | ----------------------
> | > | 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.

That is quite ok. But please rephrase the example or so, because from 
the example, tihs was quite unclear (to me).

> 
> | > | "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.
> 

Ok. Oh, these concepts ;)

> | >
> | > 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.

I'm not sure what you mean, but I think it would be worth it.
Again, I find it very misleading for CloneManager::operator() to do 
destruction.

> | > 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?

Of course. Just two more wrappers ;)

> | >
> | > 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

I might be totally off-base here, but why not an insert_clone function?

Best,
John

-- 
John Torjo
-- john_at_[hidden]
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)