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

From: Kevin Sopp (baraclese_at_[hidden])
Date: 2007-03-14 16:34:34

Hi,

>   - What is your evaluation of the design?

Outstanding, I think Ion really outdid himself with this one ;)
intrusive lib has become much more than I had ever hoped for and it's
really ready for primetime now.

It provides customizability at every imaginable point. Using
node_traits and value_traits this can be done very cleanly,
additionally one can choose between constant/linear time size() member
function. The one thing that really struck my eye the first time I
read the docs was that I could define the size type for the
containers. I never really needed something other than std::size_t but
I've read about embedded programmers that use 16 bit pointers for
small collections and so only need a 16 bit size type. Of course with
intrusive library you can use 16 bit pointers if you desire.

The one thing that still annoys me is the 'i' prefix. Really, now that
everything is in the intrusive namespace I think it should be dropped.
The first time I saw it, it reminded me of the 'I' prefix some people
use for interface classes.

>   - What is your evaluation of the implementation?

Great, it's clean and easy to understand/follow. Much simpler without
the 'accessor' layer that was present in the old code from Olaf
Krzikalla.

A couple of things I noticed after examining the library again today:
- The list of primes should be extended to 64 bit primes for 64 bit
size_t platforms
- I noticed some non-const member functions that should probably be const:
size_type bucket_size(size_type) ;
size_type bucket(const key_type &) ;
template<typename KeyType, typename KeyHasher>
size_type bucket(const KeyType &, KeyHasher) ;

>   - What is your evaluation of the documentation?

Pretty good. Can't really say more about it because I forgot my first
impressions but I noticed it has improved since december.
I agree with Samuel Debionne that the Concepts part can be improved. I
really liked the summary at the end of the Concepts section. Maybe
this should stay where it is (with one more/or a bit longer sentence
for each concept) and then put the detailed description of the
Concepts at the end of the documentation. Then the reader will know
what the traits are for and can move on. Customizing the traits is
more advanved and should be at the end.

>   - What is your evaluation of the potential usefulness of the
>   library?
>   - Did you try to use the library?  With what compiler?  Did
>   you have any problems?

Very useful, been waiting for this a long time. In fact I started to
use it back in december to implement a cache system using the
ihash_set now iunordered_set.

While this one required some boilerplate code to handle a resizable
bucket array I was positively surprised that the two functions
suggested_upper_bucket_count() and suggested_lower_bucket_count() were
provided. The library lends you a helping hand wherever it can.

Back then bucket_type wasn't copy constructible so I had to use
placement new directly, I see now that this has changed as well and I
can use uninitialized_fill().

It compiles cleanly on mingw/gcc4.1 and linux/gcc4.1.2.
I stumbled upon a compiler error in mingw/gcc4.1: I declared a name
tag for the hook in a class but upon instantiation gcc errors out with
'iunordered_set_base_hook<expression error>'. Putting the tag outside
of the class remedied the situation. I haven't examined this behavior
any further. I declare the value type that derives from the hook
inside the class too, I think this could be the reason.

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

I haven't studied the iset and imultiset containers but I've examined
the code of iunordered_set, islist and ilist.

>   - Do you think the library should be accepted as a Boost library?

Without a doubt I think it should be accepted. I believe the code is
easy to maintain and would be welcome to many programmers.

Kevin