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

From: Pavol Droba (droba_at_[hidden])
Date: 2004-10-04 05:07:31

On Mon, Oct 04, 2004 at 02:41:07AM +0200, Thorsten Ottosen wrote:
> Hi Pavol,
> 
> Thanks for your review.
> 
> "Pavol Droba" <droba_at_[hidden]> wrote in message
> news:20041003224329.GV9207_at_lenin.felcer.sk...

[snip]
 
> | 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 did see it and I find it unsufficient. I does provide the information, 
but it can be easily overlooked. Such an information should go to some
more important section and longer explanation should be gievn

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

As I already stated std algorithms should be provided along with the means
to implement the user-specific ones.

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

This is an internal issue and I see no reason why should it affect the user-level 
interface. 

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

This seems like a workaround for a functionality that can be naturaly provided
otherwise. I'd prefer null pointer.
 

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

I think, you are little be contradictive. One place you allow unsave manipulation with
the internal data one the other place you are cautious about the indirection
problems, that can be easily checked.

[snip]
 
> | >  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.
> 

I'm using arrays of smart-pointers quite often. A specific use case is a slot structure.
An array is a holder for slots. Each slot is designated by a constant (index) and can hold
a pointer to an object. It can also be empty.
There is predefined number of slots, each with a different functionality.

Imagine a set of transformations, for an image. This set can consist of 
- Bit-Depth adjustment
- Scale
- Distortion
- ...

Some of these slots can be occupied, others can be empty. I need precise indexing, so these
transforms are easily accessible from other components (like gui).

Once a transform is set, array takes the ownership of it and deletes it upon the exit.

Regards,

Pavol