From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-10-03 19:41:07
Thanks for your review.
"Pavol Droba" <droba_at_[hidden]> wrote in message
| 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
| 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
| like std::remove should not be used with the ptr_iterator. There is no
| algorithms are 'safe' and why are the others unsafe.
Did you see the FAQ entry "Which mutating algorithms are safe to use with
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
| algorithms that can be used (I have explained this in detail in the other
| All these algorithms can be provided as member functions (similary like
| remove and unique algorithms) or can be reimplemented with a help of
| 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
| 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
I actually agree; the clone manager should be broken up into the original
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() )
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
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
| for improvements.
| First, I agree with point that was already mentioned here, that several
| 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
| are very underspecified.
| I would presonaly prefer several smaller hyperlinked files to a one long
| 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
| > * 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
| > overall opinion.
| Yes, I do think that the library should be ACCEPTED as a Boost library.
| I would like to see the issues I have described to be solved, or at least
| explained in the documentation in the sufficient detail.
| > 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk