Re: [boost] Review of Intrusive library begins today March 12

From: Stjepan Rajko (stipe_at_[hidden])
Date: 2007-03-20 14:29:51

This is my first Boost review - I hope it is helpful.  Apologies if
some of the points in the review have already been addressed, I
haven't read all of the postings in detail.

> * Review questions: Here are some questions you might want
> to answer in your review:
>   - What is your evaluation of the design?

I like the clear separation of the different elements - the hook, the
container, node traits, value traits, the algorithms, etc.  Trying to
use the library for a simple task was very simple.  At the same time,
the library seems to allow quite a bit of customization through
template parameters and traits.

>   - What is your evaluation of the implementation?

I'm not very good at evalutating what makes an implementation
boostworthy.  I glanced at some code, and thought it was very clean,
concise, and well documented.  I was able to quickly determine what
the code was doing, which is not always the case with Boost libraries.

>   - What is your evaluation of the documentation?

The general documentation, tutorials on different tasks and the
examples are nicely organized and well written.  After reading parts
of the documentation, I was able to understand what the library was
for, what its advantages and disadvantages are, and a good enough idea
of how to use it.

The reference section is very detailed but hard to navigate.  After
going to the .hpp reference page, I had to figure out that I need to
click on any of the class names for more information, and then faced a
dump of all members.  Some grouping of the member functions according
to their use (even just using white space to separate them) would be
nice.  I really appreciated the complexity discussions.

Some specific comments:

Overview page:

Semantically, a Boost.Intrusivecontainer is similar
-- space missing Boost.Intrusive<->container

Presenting Boost.Intrusive containers page:

Non-raw pointers: If the wants to use smart pointers instead of raw pointers
-- If the _user_(?) wants

Cloning Boost.Intrusive containers page:

The second parameter is an function object that will clone value_type
objects returning a pointer to them.
-- is _a_ function.  Although, this sentence struck me as slightly
confusing - perhaps something like "that will clone value_type objects
and return a pointer to the clone."?
The second parameter is an function object that will destroy value_type objects.
-- is _a_ function object

Concepts page:

The word concepts seems to be getting more and more of a specific
meaning in C++... I don't know whether there is another word that
means the same thing as "concepts" but isn't "concepts", but if you
can think of one it might be good.

Class template iset page:

std::pair< iterator, bool > insert(value_type & value) ;

Effects: Tries to inserts value into the set.

Returns: If the value is not already present inserts it and returns a
pair containing the iterator to the new value and true. If the value
is already present returns a pair containing an iterator to the
already present value and false.

-- I don't think this is quite correct.  The behavior I observed is
that the value is inserted if there are no elements equivalent to it
in the set, according to the comparison function.  Not quite the same
as "if the value is not already present", I think.

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

Very useful.  I really appreciate the possibilites that it presents
and the associated advantages, as per the documentation.

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

Yes.  I tried it with the Apple's branch of GCC 4.0.1 on OS X.  I
built a couple examples and then used the library to make a little
intrusive graph.  No problemo.

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

I studied the overview of the library in the documentation, then
focused on trying to use it to make a simple graph.  I looked at the
set/multiset/unordered_set... documentation in a little more detail
and tried using it in code.

>   - Are you knowledgeable about the problem domain?

I am far far far from being an expert on containers.

> And finally, every review should answer this question:
>   - Do you think the library should be accepted as a Boost library?

Yes.  Really nice work!