Boost logo

Boost :

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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk