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

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-10-05 17:21:19

"Pavol Droba" <droba_at_[hidden]> wrote in message
news:1256163046.20040926162826_at_topmail.sk...
> Hi all,
>
> The review of the Smart Container library, written by Thorsten Ottosen
> starts today (September 26th, 2004) and runs for 10 days (until
> October 5th, 2004).

Hi,

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.

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

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.

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

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.

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.

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

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

8. For possible review issue 1, I think it's no big deal that you can't use
std::stack to adapt a ptr_container.

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.

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.

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

1.  Clonable is first said to refine Copy Constructible, but then the docs state
"a type T might be Clonable even though it is not Assignable or Copy
Constructible "

2. Under the heading "Clone Manager", it looks like 1. and 2. might be reversed;
at any rate it's not clear.

3. The Clone Manager notation table should be put in the canonical form, e.g.,

    CloneManger - A type modeling Clone Manager
    cm, cm2 - Objects of type CloneManager

4. I'd prefer the acknoweldgements to say something like this:

Jonathan Turkanis for supplying his move_ptr framework -- used internally by the
library -- based on the work of Rani Sharoni, Daniel Wallin, Howard Hinnant and
Bronek Kozicki.

5. In first line of indirect_predicate.html, 'what' should be 'want'.

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

7. Under clonable concept, you say " If you are implementing a class inline in
headers, remember to forward declare the functions ". You should elaborate on
this.

8. In the first line after the heading "Terminology", the phrase 'an
heap-allocated object' should probably be 'a heap allocated object'. 'an heap'
sounds schoolmarmish.

9. The first example is way too long and is not a good introduction to the
library. In fact, it's the reason I delayed writing a review until the last
minute. A tutorial section should start by explaining the problem to be solved,
and then show step by step how to solve it. One long block of code, even
commented code, is too much to digest when one knows nothing about the library.

10. British and American cows say "Moo".

11. Sound that pigs make, according to google:

  oink - 160,000 hits
  oiink - 626 hits
  oiiink - 85 hits
  oiiiink - 15 hits
  oiiiiink - 18 hits
  oiiiiiink - 4 hits
  oiiiiiiink - 4 hits
  oiiiiiiiink - 1 hit
  oiiiiiiiiink - 5 hits
  oiiiiiiiiiink - 2 hits
  oiiiiiiiiiiink - 3 hits
  oiiiiiiiiiiiink - 3 hits
  oiiiiiiiiiiiiink - 1 hit
  oiiiiiiiiiiiiiink - 3 hits
  oiiiiiiiiiiiiiiink - 0 hits
  oiiiiiiiiiiiiiiiink - 36 hits
  oiiiiiiiiiiiiiiiiink - 1 hits

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

Extremely useful.

>     * Did you try to use the library?  With what compiler?  Did you have any
problems?

No.

>     * How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

About three hours.

>     * Are you knowledgeable about the problem domain?

Yes.

Best Regards,
Jonathan