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

From: Dave Harris (brangdon_at_[hidden])
Date: 2004-10-09 07:46:55

In-Reply-To: <797960166.20041007113153_at_[hidden]>
droba_at_[hidden] (Pavol Droba) wrote (abridged):
> Formal review of the Smart Containers library from Thorsten Ottosen
> has been extended until Saturday, October 7th 2004).

Goody. Here are my comments.

>     * What is your evaluation of the design?

I felt it was a bit muddled in its objectives. For example, it presents 
itself as a "container of pointers to heap-allocated objects", but then 
does not use pointer-like syntax or support NULL pointers. It offers both 
begin() and ptr_begin().

I feel there are really two kinds of container here. The first is a 
container of pointers which takes ownership of the objects it points to. 
This would be a more-or-less drop-in replacement for vector<T*>. It would 
allow nulls, and syntax like (*v.begin())->method().

The second is a container of values which may be polymorphic. It would be 
a more-or-less drop-in replacement for vector<T>. It would not allow 
nulls, and the syntax would be like v.begin()->method(). Although it would 
probably use pointers behind the scenes, that would be an implementation 
detail not revealed by its interface.

The present submission has tried to combine these two kinds of container. 
I believe they would be better kept separate and given different names. 
I'd suggest "owning_vector" and "poly_vector", or similar, since the one 
is about ownership of pointers and the other is about solving the 
polymorphic value problem.

Incidently, I also dislike the library calling itself "smart" because that 
word is too vague. Smart in what way? 

I am bothered by the libraries interaction with std algorithms. For 
example, std::remove_if<> doesn't work with the container's iterators. 
That makes it, at best, a low-level library that needs to be managed 
carefully. 

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

Why not use vector<shared_ptr<T> > instead? That provides a simple 
out-of-the-box solution without the problems with std algorithms.

The present library claims performance benefits, but if I need those I'd 
write my own wrapper for vector<T *> which did exactly what I needed and 
no more. A library which made that easier might be useful to me, but I 
don't think the present submission is that library.

The library also claims better syntax, but for me that benefit is 
negligible. So I don't believe I will ever use this library.

>     * What is your evaluation of the implementation?
>     * What is your evaluation of the documentation?
>     * Did you try to use the library?  With what compiler?  Did you 
> have any problems?
>     * How much effort did you put into your evaluation? A glance? A 
> quick reading? In-depth study?
>     * Are you knowledgeable about the problem domain?

I have not looked at the implementation or attempted to use the library. I 
read the documentation, got confused, got unconfused via this list, and 
then decided it was not for me. Maybe half an hour of effort. I don't 
expect my opinion to carry much weight.

I use std containers, and have often written wrappers for containers of 
raw pointers.

>    * Do you think the library should be accepted as a Boost library?

No. I strongly object to the library with its present naming.

If it were renamed along the "poly_vector" lines, I would probably be 
neutral.

-- Dave Harris, Nottingham, UK