[boost] Re: Yet another Variant class

From: Itay Maman (itay_maman_at_[hidden])
Date: 2002-06-28 12:50:04

Douglas Gregor wrote:
> On Friday 28 June 2002 06:03 am, Itay Maman wrote:
> 
>>It seems that the variant issue is very popular these days ... :)
>>
>>The last couple of weeks I've been implementing a variant class
>>based on the ideas (which were, in large part, contributed by Doug
>>Gregor) discussed here in "Proposal --- a type safe union"
>>
>>The main advantage of this variant implementation is the flexibility of
>>its visitor mechanism, which accepts 'standard' function objects. Having
>>this flexibility at hand, the implementation of other key-features (e.g:
>>recursive types) became fairly simple.
>>
>>The source code (with two short programgs) is available at
>>http://groups.yahoo.com/group/boost/files/variant_b.tar.gz
> 
> 
> Very nice. It's quite a bit shorter than I had expected. I have a few specific 
> comments on the interface and implementation, but overall I'm very happy with 
> it.
> 
> Interface comments:
> 
> 1) get_ptr: first of all, is this meant to be part of the public interface? If 
> so, I think it might be more intuitive to use explicit specialization to get 
> the type instead of exposing 'Type_to_type' to the user, e.g., use  
> v.get_ptr<T>() instead of v.get_ptr(Type_to_type<T>()). The alternative 
> (which should probably exist anyway) is to have a variant_cast that acts like 
> any_cast.

I think that variant_cast<> should be the only way for an application 
code to reach the T* pointer. Consequently, get_ptr() should be made 
private, although I must add an #ifdef-ed workaround for MSVC.

> 
> 2) The default constructor is dangerous - it says that the value has tid 0, 
> but doesn't set any value. I think the default constructor should 
> default-construct the first type (that way, it is impossible to have a 
> variant that has no value in it).
> 

Well, It's already there. Stack_holder's default constructor initializes 
itself with the default value of the first type (t_first).

> 3) Is the 'assign' member function meant to be public? It's in the public 
> interface, but it does not handle variant-to-variant copies like the 
> assignment operator or constructors do.

Again, I agree with you. It should be private.

> 
> 4) The Variant class takes a typelist parameter. We discussed this a bit, and 
> there were three options:
>   a) Always take a typelist parameter (e.g., Variant<mpl::list<t1, t2, t3> >)
>   b) Always take types as separate parameters (e.g., Variant<t1, t2, t3>) 
>   c) Allow syntax from both a & b
> 
[snip]

Well, we did discuss that and I believe we have agreed on option c). 
However, I didn't want to this version of the code to support this 
feature, primarily due to simplicity considerations.

> Implementation comments:
> 
> 1) Great Stack_Holder::swap :) 

I am wondering if there's a more efficient way for implementing an 
exception-safe swap() ?

> 
> 2) Why the const_cast in assign_variant_to_variant? I'd think it should keep 
> the source of the copy const, always, but non-const values are always used 
> for the copy?
> 
> 3) In Assign_helper::set_item, the value passed in should probably be a passed 
> as a reference-to-const instead of a copy.

Both points are corrects.

> 
> 4) Is there any benefit to the use of copy/destroy function pointers instead 
> of a cascading 'if' built with a metarecursive function? The function 
> pointers impose an extra 8-byte overhead in the variant -- is this overhead 
> justified by an efficiency boost? The same question applies to the visitor 
> table.

Well, we can use a single pointer for *both* clone/destroy. So the space 
overhead we should weigh here, is just 4 byte.
As for Accept_table, it is a static local variable inside 
Variant::operator() so its cost is minimal.

-Itay