Boost logo

Boost :

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

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.


Boost list run by bdawes at, gregod at, cpdaniel at, john at