Re: [boost] Last minute intrusive review

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2007-03-21 12:58:33

Hi Daniel,

Daniel James wrote:
> 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.

Thanks. Maybe the use of Intrusive to model relationships can be 
converted in a new Boost library.

> 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.

Ok. Something like a "quick guide for the impatient".

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

Ok. But I'm thinking that maybe list_algorithms and slist_algorithms 
should be called circular_list_algorithms... because maybe in the future 
algorithms for null ended list could be added. Just a thought.

> 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.  

Well, i suppose that your "smart pointer" will have 3 pointers, one 
being the actual pointer to the pointee and the other two to form the 
list. You can use list_algorithms to do what you describe (add a node to 
a group of nodes and remove a node from a group of nodes without having 
a reference to the group. You just need to define your NodeTraits 
compatible with these algorithms. Intrusive could also define a simple 
node and its NodeTraits class, so that you can use the algorithms 
directly without defining anything.

> 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.

Ok. Already mentioned and I agree that it's more self-describing.

> 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" prefix will disappear, don't worry

> 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).

Ok. Another place for improvements.

> 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;

Well, you save the "::value_traits" part so I'm not sure if that's a big 
improvement.

> 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.

I will think about this. Maybe the common cases should be simpler, but 
for every container you need to specify the value type and the hook the 
container will use. For member hooks there is no default possibility, 
because the offset of the member can be unknown. With base hooks, maybe 
the situation can be simpler, because the transformation between the 
hook and the value type is based on static_cast.

> 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.

Ok. A default tag sounds good.

> Daniel

Regards,

Ion