[boost] Re: [review] Formal review of the Smart Containerlibrarystarts today

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-10-05 18:32:35

Hi Jonathan,

Thanks for your review.

| Here's my review. I believe the Smart Container library should be ACCEPTED.
I
| have some suggested modifications, but there is only one issue which I think
| needs to be resolved before I can give the library my unconditional approval
| (item 5 under "evaluation of the design"). Since I did not have time to
study
| the implementation, my yes vote depends on other reviewers having done so
and
| found it acceptable.

Thanks...am sure we can solve those issues.

| >     * What is your evaluation of the design?
|
| Very good, on the whole. I have these comments (in no particular order):
|
| 1. The functions used in the definition of Clonable are not well named. I
would
| prefer either
|    - allocate_clone/deallocate_clone or construct_clone/destroy_clone, to
mimic
| the interface of standard allocators, or
|    - something using new/delete, as someone else suggested

yeah, there is some inconsistency here.

| 2. It's a bit odd that CloneManager uses operator() to destroy objects. I
| understand that this is a requirement of using CloneManager as a deleter,
but
| you could easily use an adapter instead.

ok.

| 3. I think Cloner sounds better than CloneManager. Cloner is already an
English
| word; CloneManager sound like a mixed metaphor.

As I mentioned in another mail, and as sugeested by Pavol, it would probably
be better to
rip CloneManager apart into Allocator and CloneAllocator

| 4. Since performance is one of the main design goals, I'm suprised that
| attempting to insert a null pointer causes an exception rather than an
insertion
| failure. The same goes for invoking the functions for back, front, pop_back
and
| pop_front with an empty container. By coincidence, this morning I was
| experimenting with some code that invokes a function pointer with a pointer
as
| argument, like so
|
|     f_(p_)
|
| in an inner loop. I tried replacing the above code with
|
|    if (p_)
|       f_(p_);
|    else
|         throw std::runtime_error(...);
|
| I found an enormous slowdown, which should be no surprise.

yeah, I dunno, this might depend heavily on the compiler. what compiler do you
use?

At least for back()/front() you can avoid a check by
*ptr_begin()/*--ptr_end().
For vector/deque you can unchecked random access too via operator[]().

It is is true that performance was a design goal, but so was safety. The
domain of pointers
allowed me to do different trade-offs than with standard containers. For
example, does
it really matter if container.push_back( new T ) throw if 0 was inserted? IMO
no, because
the heap operation will dwarf any other operation by several magnitudes.
The same considerations would be true for eg. pop_back() when calling the
detructor
on the object since we need to remove the object from the heap.

| 5. The caveat "splice()/merge() in ptr_list is not thought through yet"
makes me
| very nervous. What aspect needs to be thought through? Does the current
| implementation have well-defined semantics? I have not examined the
| implementation, so I can't answer these questions, but whatever the caveat
| means, resolving it should be a condition of acceptance.

yes, I understand your concern and I promise to find a solution to this. It's
just that
each function needs to be faily carefully examined to see if its specs can
potentially lead to leaks
and other problems. I just didn't have the time, and I thought the review
could work without people
worriying too much about splice().

| 6. To answer possible review issue 13, I'd say you should parameterize the
| provided clone managers by allocator type, defaulting to std::allocator<
| [unspecified] >.

why unspecified instead of T* ?

| 7. I don't understand review issue 10. I don't see the erase()/insert()
| functions that return pair<iterator, bool>.

in set/map and ptr_set/ptr_map these functions returns this pair to say "we
did/did not insert the element".

I think that the same should hold for transfer; it would be only natural.
Another way would of course to return
optional<iterator>.

| 9. For possible review issue 5, theoretcially I'd like to see auto_type
exposed
| as move_ptr. (I now like Howard Hinnant's suggested "unique_ptr", though he
| seems to be leaning toward "sole_ptr" now.) Then you could also use move_ptr
| where you now use auto_ptr. However, I agree with one of the other reviewers
who
| said that this would really be appropriate only if move_ptr were a
stand-alone
| library. I haven't proposed move_ptr mainly because I'm not sure whether it
will
| be made obsolete by policy_ptr and because while there was a great deal of
| discussion in January, there wasn't much comment after I posted my recent
| version.

yeah, we'll wait and see.

| 10. If ptr_iterators are kept, I agree that there needs to be a much more
| detailed explanation of which algorithms they can be used with.

ok.

| >     * What is your evaluation of the implementation?
|
| The parts I looked at looked good, but that means little since I only gave
it a
| quick glance.
|
| >     * What is your evaluation of the documentation?
|
| It was hard to find my way around in the documentation. In general, there
should
| be a better index and more hyperlinks. My javascript menu is now a de facto
part
| of boost, so you might consider using it ;-)  Here are some more detailed
| comments, in no particular order:

yes, good idea.

[I have snipped most of your detailed comments, but I will look into them]

| 6. On mozilla, some of the preformatted text overflows the bounding box,
e.g.,
| in "3. Copy-semantics of smart containers"

hm...yeah, I don't really know why it does that.

| 10. British and American cows say "Moo".
|
| 11. Sound that pigs make, according to google:
|
|   oink - 160,000 hits

:-)

| >     * What is your evaluation of the potential usefulness of the library?
|
| Extremely useful.

Thanks!

br

Thorsten