[boost] Last minute intrusive review

From: Daniel James (daniel_james_at_[hidden])
Date: 2007-03-20 21:55:14

Hello,

Sorry, this is review is a bit late and rushed. I feel like I'm back at  
college finishing a paper the night after it's due...

I've read through the documentation and looked at some of the  
implementation (mostly the unordered containers).

I think Intrusive should certainly be added to Boost. The design seems  
very flexible and anticipates most of the use cases that I can think of.  
Intrusive data structures can be useful. As well as the potential memory,  
performance and exception safety gains, they are perhaps a more natural  
way to model the relationships between entity types than containers of  
pointers.

The documentation is good. But I think it could be improved if it showed  
complete code examples much sooner - at the moment it goes into the  
concepts in some depth before the reader gets a feel for how they fit  
together. I think it's possible to show how to make basic use of the  
simpler intrusive containers and then ease the reader into the more  
involved details.

It's specified that islist and ilist are implemented as circular lists -  
this is an implementation detail and doesn't need to specified.

It might also be useful to have intrusive structures without container  
types. Say you were implementing a smart pointer where all the pointers to  
the same object are linked together by a circular list. The copy  
constructor has a pointer would add the new pointer to list, the  
assignment operator would remove the pointer from its current list, and  
add it to the new one. All of this would be done without ever referring to  
a container. And it's not clear who'd be responsible for the container.  
This would require the hooks to be more complicated (I guess they would be  
something more than just hooks). And they'd probably need to use the  
curiously recursive template pattern. But maybe this is outside of the  
scope of the library.

I think this may have already been mentioned by someone else, but the  
true/false template parameter (for constant time size) isn't very clear.  
In such cases, I always forget which is true, and which is false - and  
that's if I can remember what the parameter stands for in the first place.  
I think it'd be clearer to use a couple of appropriately named types.

And I agree with other people's dislike of the 'i' prefix - it's already  
used in the string algorithms for 'case-insensitive'. Also ilist and  
islist look a little too similar.

I had a quick look at the tests and it looks like there could be some more  
(which is of course always true). Exception tests could be useful where  
needed (Boost.Test has a pretty good exception testing framework, but it's  
not documented).

Finally, something which I haven't spent enough time on. And it's late. So  
I'm probably saying something really stupid. But basic containers seem a  
little too complicated to define. Especially for dependant types inside  
templates.

Something like:

typedef islist< islist_base_hook<my_tag>::value_traits<MyClass> > BaseList;

Could perhaps be:

typedef islist<MyClass, islist_base_hook<my_tag> > BaseList;

With a third parameter that could used to specify an alternative  
value_traits when needed. Which I think fits in better with the typical  
STL way of doing things. It might be possible to do something simpler than  
this for some special (but common) cases - but the only ones I've thought  
of so far are problematic.

It does seem odd to have to sepecify 'islist_base_hook' for 'islist' - I'd  
expect that to be the default. I guess to elide that there would need to  
be some mechanism to distinguish between tags and hooks - allowing you to  
use just the tag. A bit iffy.

Also, a default tag would be useful. Most of the time a container will  
only have a single hook of a certain type and declaring a tag in this case  
is unnecessarily verbose.

Daniel