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

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

Hi Pavol,

Thanks for your review.

"Pavol Droba" <droba_at_[hidden]> wrote in message
news:20041003224329.GV9207_at_lenin.felcer.sk...
| 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.

you're welcome.

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

ok, noted, but I dont agree.

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

Did you see the FAQ entry "Which mutating algorithms are safe to use with
pointers?"?

I could of course go through every mutating algorithm in the standard library,
but then we would still
miss all other algorithms in boost or custom made.

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

the problem with this is AFAICT scalability. which algorithms do we consider
essential and what guarantee do we have that
others do too?

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

First, operator()() cannot be static.

Second, operator()() is provided because a clone manager act as a deleter
passed to the internal move ptr
and other classes.

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

I actually agree; the clone manager should be broken up into the original
Allocator
and a CloneAllocator policy having ::clone() and operator()() members.

| * Current implementation is restricted to non-null values. Still I think,
that null support
|   could be useful.

don't you like the null object pattern?

|   Few functions for checking like is_null(index) and iterator.is_null()
could handle do job.

I think the core working group is going to allow null references, which means

if( &*begin() )

becomes legal.

See also active issue "232. Is indirection through a null pointer undefined
behavior? ". From the latest discussion:
"
Notes from the October 2003 meeting:

See also issue 315, which deals with the call of a static member function
through a null pointer.

We agreed that the approach in the standard seems okay: p = 0; *p; is not
inherently an error. An lvalue-to-rvalue conversion would give it undefined
behavior

"

However, it is true that one reason for the containers to not allow 0 is to
avoid troubles with 0 indirection on iterators.

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

indeed :-)

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

ok.

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

yeah, some is already hyperlinked, but I guess more can be put into seperate
files.

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

Thanks!

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

that is what makes ptr_array a bit different; it needs to make its constructed
state 0; I'm not convinced that
allowing 0's is any benefit in general, but I'm open to is_null( index ) and
is_null( iterator ) predicates.

br

Thorsten