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

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-10-03 07:55:46

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?

| 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