Re: [boost] [intrusive] Review

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2007-03-20 13:42:23

Guillaume Melquiond wrote:
> Quoting Ion Gaztañaga:
> 
>> template<class T, member_hook T::* P>
>> class value_traits;
>>
>> compiles fine, but I just want to have one template parameter
> 
> This is not a good enough rationale for the current interface. I insist in the
> need for moving the class parameter from the hook to the value traits, 
> both for
> efficiency and clarity.

I'm not trying to justify the current interface. I just wanted to say 
that two parameters will be needed. It's clear that the hook shouldn't 
have a template parameter because we are generating unnecessary code. 
Thanks for pointing this out.

> I hadn't noticed that your functions size() and count() don't return a 
> size_type
> as defined by the standard. The standard says that the return type of these
> functions must be large enough so that it can contain any positive 
> value of the
> pointer_type of the iterators of your containers.
> 
> Moreover, there is not much point in putting a short type as an integer return
> type, as the value will generally go through a fixed register, which doesn't
> care about the reduced size of the return type. It may even produce worst
> binary code, as either the callee or the caller will have to explicitly 
> discard
> the upper bits.

The standard also allows allocator::size_type to be any type, so if I 
want to support building STL containers above Intrusive, I must somehow 
allow specifying the size_type for Intrusive.

> But it doesn't have to be properly unlinked for it to be perfectly safe and
> meaningful. As a matter of fact, you want to be able to change an element of a
> list; they are not supposed to be immutable. Your library doesn't allow a user
> to do "*i = v" when i is an iterator of a list and v an unlinked value. This
> really strikes me as wrong!

"*i = v" is convincing. You can still achieve this if you define 
operator=() for a value_type, not calling hook::operator=(). However, I 
agree that that compiler generated assigment operator should allow "*i = 
v". So I will make hook assigment a no-op, to maintain the value linked 
in the container. This will make ordered containers a bit unsafer, but 
you can't have everything ;-)

> Maybe it would be clearer with an example. The following pseudo-code is
> something I want to be able to write.
> 
> struct T { hook1; hook2; data };
> iset<T,hook1> set1;
> iset<T,hook2> set2;
> ...;
> set2 = set1;

If sets are different types, operator= has no sense in my opinion, 
because it should destroy old values and there is no function to do so. 
To me, only a move assignment/construction (in C++0x) would make sense:

set2 = std::move(set1);

which would be equivalent to a "set2.swap(set1)" (no value is 
destroyed). But intrusive containers are non-copyable and non-assignable 
and I want to emphasize this with the interface. A matter of taste, I guess.

> Best regards,
> 
> Guillaume

Thanks again,

Ion