Re: [boost] Yet another Variant class

From: Douglas Gregor (gregod_at_[hidden])
Date: 2002-06-28 09:38:54

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.

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

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.

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

  Here's my take on the relative merits of each interface (feel free to 
correct, expand, annotate, etc. Whatever we choose we should document well).

  a) Pros:
    i) Typelist can be of unlimited length
    ii) Easy to use variant when a metaprogram determines the list of types

     Cons:
    i) Requires users to learn typelists (even for simple uses)

  b) Pros:
    i) Simple syntax for simple uses
    ii) Typelists are an implementation detail

     Cons:
    i) Number of types in a variant is limited
    ii) Harder to use variant when a metaprogram determines the list of types 
(need to be able to 'unpack' a typelist into a bunch of types passed to 
variant)
    iii) Error messages will include all those extra template parameters that 
nobody cares about

  c) Pros:
    i) Everything from a and b :)
    
     Cons:
    i) Can we always tell what's a typelist and what isn't? Does something 
like boost::tuple, which is both a typelist and a data structure, confuse the 
variant class?
    ii) Overloading might confuse users (though overloading to this degree is 
generally understood by C++ programmers)
    iii) Error messages will include all those extra template parameters that 
nobody cares about

  
  My proposed solution: First of all, I put a _lot_ of weight on the 'simple 
syntax for simple uses' argument. I think users can live long, happy 
programming lives without ever seeing a typelist. However, I know that there 
are cases where typelists of unknown length will be used in variants, so at 
some point we need to support that. For this reason, I think we should go 
with 'c' and allow both syntaxes, but in a restricted manner. The template 
signature would look something like this:

  template<typename TypelistOrT1,
           typename InvalidOrT2 = unused,
           typename InvalidOrT3 = unused,
           ...
           typename InvalidOrT4 = unused>
  class variant;

  So this says that a syntax like 'variant<mpl::list<int, float, double> >' is 
okay, as is 'variant<int, float, double>', but an ambiguous instance like  
'variant<tuple<int, float>, double>' is resolved by considering the tuple 
argument as a normal type argument and not a typelist. Essentially, the 
second template parameter (InvalidOrT2) is how we determine if the first 
parameter is a typelist or a variant: if InvalidOrT2 is 'unused', then the 
first template parameter must be a typelist and we ignore all other 
parameters. If 'InvalidOrT2' is anything else, then the first template 
parameter is a normal type, and we should construct the typelist from all of 
the template parameters. This solves the first 'con' of option 'c', because 
we don't have to know if a type is a typelist or a normal type, or both; 
instead, we use context information. Granted, this makes something like 
'variant<int>' illegal, but I don't see any harm in making useless, 
inefficient code illegal.

Implementation comments:

1) Great Stack_Holder::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.

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.

        Doug