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