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

From: Pavol Droba (droba_at_[hidden])
Date: 2004-10-03 17:43:30

Hi,

First of all, I'd like to thank Thorsten for bringing up this library. 
It implements a very common programming pattern that is currently
missing in STL or Boost.
 
>     * What is your evaluation of the design?

I find the design sufficient and reasonable for the purpose of the library.

There is only one important issue, that I find problematic (I have already mentioned it in 
the other mail).

The issue is about the direct write access to the contained pointers, via the ptr_iterator.
Using this interface, all invariants of the underlying container can be broken. It is not
even possible to validate the problem. Therefor I suggest to remove it, unless it can
be fixed.

Still this access can be usefull from time to time, but I simply don't think, that it
is worth the problems, it can bring.
Documentation is not very clear about the problems. It merely notes, that algorithms
like std::remove should not be used with the ptr_iterator. There is no explanation which
algorithms are 'safe' and why are the others unsafe.

After small analysis I have found out that there is actualy a very narrow set of
algorithms that can be used (I have explained this in detail in the other mail).
All these algorithms can be provided as member functions (similary like current
remove and unique algorithms) or can be reimplemented with a help of swap_element(size_type, size_type)
function (or something similar).

Other small issues:

* CloneManager::operator(). 

  I don't like this notation. I would prefer something more explicit. I see no clear connection 
  between this operator and the operation it provides.

  Also I don't understand the asymetry between CloneManager::clone function and operator().
  Former is static, while the later is member function. Why?

* I would like to see explicit allocator specification for underlying container. There were good
  reasons to provide it in the STL and from the same reasons I'd like to see it here.

* Current implementation is restricted to non-null values. Still I think, that null support
  could be useful. 
  Few functions for checking like is_null(index) and iterator.is_null() could handle do job.

>     * What is your evaluation of the implementation?

I find the implementation good enough to be included in the boost.

>     * What is your evaluation of the documentation?

Documentation is sufficient for the understanding the library, but there are several posibilities
for improvements.

First, I agree with point that was already mentioned here, that several small examples
are better a long one. Introductory example is too long to be easily comprehended in the
first reading.

There is lot of rules and guideling that are explained only in a few sentences. Some of them
would greatly benefit from a small example and others (like the one about 'safe' algorithms) 
are very underspecified.

I would presonaly prefer several smaller hyperlinked files to a one long document. Although
it is easier to print it, I found it harder to read it in the browser.

>     * What is your evaluation of the potential usefulness of the library?

I think, that the library is very usefull. As I said it implements very common pattern, that
is not sufficiently solved otherwise.

>     * Did you try to use the library?  With what compiler?  Did you have any problems?

I have not compiled the library.

>     * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

Few hours of reading the documentation and code and thinking about the possible issues.
Also I spend some time in discussion with the author.

>     * Are you knowledgeable about the problem domain?

This library deals with a common problem. I consider myself knowledgeable in this domain. 

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

Yes, I do think that the library should be ACCEPTED as a Boost library. However
I would like to see the issues I have described to be solved, or at least
explained in the documentation in the sufficient detail.

Here are my answers for some questions asked in the documentation:

>  1. The reference type, SmartContainer::reference is indirected. This has the unfortunate consequence that eg. 
>     we cannot use eg. ptr_vector with std::stack. The question is if its worth adding another policy just to achieve this.

      Considering the library purpose, I think, that the current state is sufficient.     

>  2. Is the ptr_array class worth the trouble?
      
      I would like to see it. Especialy if the null-value can be supported.

>  3. The clone manager effective removes allocators from the container template parameter interface. 
>     Can people who want to use allocators still do what they want?

      Yes, I'd like to see it there.
      
>  5. Should SmartContainer::auto_type expose itself as a move_ptr?

      Unless the move_ptr would became a self-contained utility, possibly as another library, I think it should
      stay like this.

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

      Boost.Range is already a part of boost. I think that the later interface is obsolete now.

> 10. should transfer() return iterator or iterator/bool pair like erase()/insert() do.

      For map, transfer has similar semantics to insert. I think, that interfaces should be similar as well.

> 12. should we provide member algorithms for unsafe standard algorithms like unique() and remove?

      Eithter provide them as member function, or provide a means for standalone implementation.
      In any case it is wise to provide some functional variant.

Best Regards,

Pavol