Re: [boost] Re: [review] Formal review of the Smart Container librarystarts today

From: John Torjo (john.lists_at_[hidden])
Date: 2004-10-04 11:22:17

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.

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

Besides, there are operations which don't make sense for a "null object".
And, for each new (virtual) function you add, you should update the 
null_object class - which I think can turn into a maintenance nightmare :(
> | ---------------------
> | 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.
> 
:) look forward to 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.

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

Well then, say so ;)
Something like "add constructors here (optional)" - or something.

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

yup.

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

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

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

oh yes! I see the light now ;)
You're totally right.

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

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)