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

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2007-03-13 17:14:14

Hi Samuel,

Samuel Debionne wrote:

> My first conclusion : Boost.Intrusive is an interesting library to me.
> Let see more in details.

Glad to hear it.

> - What is your evaluation of the design?
> The use of hooks is user-firendly and I appreciate to be able to choose
> between member or base class hooks. Auto-unlink hooks are also intersting
> but I did not have a try. The instantation of a container was not a
> problem neither.
> In the other hand, I didn't not inderstand why the containers implements
> their own algorithm (reverse, remove_if, unique...) as member function.
> Are there any reasons not to use their STL counterparts ?
> Some of my comments overlay the documentation review, so see below.

Well, the aim of Intrusive was to offer the same interface as their STL 
counterparts. Unique is more efficiently implemented as a member 
function and if you find std::list::reverse useful, I'm pretty sure 
boost::intrusive::ilist::reverse will be useful too.

> - What is your evaluation of the implementation?
> I didn't need to look at the inside of the library that much... which is a
> good sign for a end-user review. Just one question : iterators couldn't be
> implemented with iterator_facade ?

Good question. I wanted to minimize Boost.Intrusive dependencies, but I 
have no objection to use iterator_facade.

> - What is your evaluation of the documentation?
> The overall documentation is fine. It is written with QuickBook. About the
> shape, I would put the usage before the concepts. And group "When to use"
> together with the usages.

Ok. Usage before concepts will make the documentation friendlier for a 
first-time reader.

> The text describing the concept is not what I expected and remains a bit
> obscur. The concepts seems to be mixed with some implementation details.
> In my opinion, this part should define first the different nodes concepts
> (i.e. NodeSinglyLinked, NodeDoublyLinked...) that are the first template
> argument of the containers.

Ummm. I tried to do my best, but I agree that part can be improved.

> The hooks are only helpers to make a class a model of these concepts.
> Value traits is a also a helper, a "hook selector/adaptor", for classes
> that model multiple nodes. They are not really concepts.

Well, the problem is that they are mentioned several times, so at least 
they should be defined.

> As far as I understand, node algorithms are also an implementation detail
> that enable to share the basic mecanism of the container through several
> instantion of the same container with different types (static function). I
> would move this part to the design notes.

My intention was to offer the node algorithms as a public part of the 
library so that library writers in need of low-level node algorithms 
could find them in Intrusive. For example, algorithms for a red-black 
tree, which are not trivial to write. But if reviewers, consider this an 
implementation detail, I'm ready to remove it. The problem is that if a 
programmer wants to use Boost.Intrusive with his own "rare" nodes (as 
presented in the "Containers with custom ValueTraits" section) he needs 
to write the NodeTratis and ValueTraits. And these should be explained 
somewhere.

> Did you consider using Boost.Concept to check the template parameter of
> the containers ?

No. But I will look at this. If this does not result in large 
compilation times or bigger code size, I'm for it. But I want to 
preserve Boost.Intrusive as lightweight as possible. My goal is that 
Boost.Intrusive should be one of the first libraries embedded C++ 
programmers look for.

> Finally, the different container and iterators concepts should be defined
> (when they differ from STL, see bellow).

Ok. This is quite a big job and I would be interested in receiving 
feedback about "how" these concepts should be defined (another chapter, 
comparison with STL concepts...). Good comment, anyway.

> In other places in the doc, some requirement (i.e. smart pointer
> requirement = TrivialIterator + get_pointer()) could be expressed as
> concept or refinement of existing boost or slt concepts.

Ok. Concepts are the key, I'm afraid ;-)

> - What is your evaluation of the potential usefulness of the library?
> I find it usefull and a good complement to their STL counterparts. As the
> authors stated, I agree that this library is more a building block to
> build other more powerful, encapsulated containers. To be used carefully.

Thanks.

> - Did you try to use the library?  With what compiler?  Did you have any
> problems?
> Yes. I used the library with msvc8. No I didn't have any problems with the
> library itself. But yes, I add some with its interaction with the STL.
> Intrusive containers claim to offer an STL-like interface which is not
> entirely correct.

I agree. The problem is what "STL-like" means. Obviously, the constness 
of the insertion objects is going to break some STL code. But you still 
have your iterators and all other algorithms. I agree that I should 
stress the problems we can have with insertion functions.

> Here there are two options : either Intusive containers does not model the
> BackInsertionSequence concept and that should be written in the doc or/and
> some adapted insert iterator should be provided by the library.

Let's do both. Thanks for your example. So I'm afraid that intrusive 
won't match any STL concepts.

> - Do you think the library should be accepted as a Boost library?
> Yes with minor corrections.

Thanks for you vote!

> I would like to thank very much the authors for their great work.
> Hoping that it will help,

Sure. Thanks for the review.

> Samuel

Regards,

Ion