Boost logo

Boost :

From: Eric Friedman (ebf_at_[hidden])
Date: 2003-02-17 21:40:57

Ed Brey wrote:
> I vote that variant be accepted into boost. I read all the
> documentation, and tried out the code in a simple test under
> VC7. I am very pleased with this library. Following are a
> comments I have that can help make it even better:

Thanks for the favorable review. I'll try to address your comments

> *Design:*
> Please consider incorporating a "blank" type. This idea
> would be to allow the equivalent of "void" to be added to the
> type list. boost::blank (or whatever it would be called)
> would be meet BoundedType concept, but otherwise do nothing.
> The empty function would return true if a blank is currently stored.

This is an interesting idea. It may even be possible to implement
variant to detect a void type so that the following would be allowed:

  variant< T1, ... void, ... Tn >

In the case of a variant "containing" void, empty() would return true,
and visitation would be undefined.

I'd be interested if others have opinions on this issue.
> Interestingly, with the addition of a blank type, variant
> comes close to being a superset of boost::optional, although
> not quite close enough to obsolete it.

Yes, not quite enough to obsolete it: as I noted to Fernando, variant
carries more implementation baggage than boost::optional. However, the
connection is conceptually sound.

> Extract is confusing. One problem is that it is deceivingly
> named. It doesn't extract data from the variant at all, but
> rather provides type-specific access to data that still
> resides in the variant. It is not clear from the name or the
> documentation that this would be bad:
> variant<int>* v = new variant<int>;
> extract<int> i(*v);
> delete v;
> return i;
> The area would be helped by renaming extract to access.

I tend to agree the name is confusing. So shall we call it
boost::access<>? Input?

> The other point of confusion is when bad_extract gets thrown.
> The usage sections actually does more good than harm. When
> I read it, I first thought that under some conditions the
> constructor would throw and sometimes it wouldn't. The throw
> at in the call to operator() was subtle, especially since the
> call isn't even needed. It would be better to skip the usage
> and state in the constructor section that the constructor
> never throws.

Duly noted. If the extract (access?) facility survives the review, I'll
make the change to the docs.

> Another approach to the throwing problem would be to
> eliminate the class altogether, and instead provide a member
> function that returns a reference to the desired type, or
> throws if there is a type mismatch. The reference would be
> invalidated by an assignment of a different type to the
> variant, but that is only an incremental restriction, since
> the reference is invalidated in any case by destruction of
> the variant. Likewise, it would be reasonable to provide a
> non-throwing function (perhaps called access_matching) that
> returns a pointer. I know these simple member functions
> sound mundane, but I think they will solve the problem well.

I don't think a member function really will solve the problem, but I do
agree the whole extract facility can be improved.

In the past we sought to support the following...

  variant<...> var;
  T* p = extract<T>(&var);
  T& r = extract<T>(var);

...but it had to be dropped because (essentially) ambiguity exists
between the following:

template <typename T, typename Extractable>
  T& extract(Extractable & operand);
template <typename T, typename Extractable>
  T* extract(Extractable * const operand);

If someone has an idea for a better solution, Itay and I would certainly
welcome it.

> Finally, it wasn't clear to me why the return type for
> which() wasn't unsigned.

It's not? I see that which() returns an unsigned int, which I believe is
the same as unsigned. (Am I wrong?)

> *Implementation:*
> The destroyer class contains a function with an unreferenced
> formal parameter, which triggers a warning under VC7. Since
> this a useful warning, all unreferenced formal parameters
> should be removed (or commented out).

I'm not sure what parameter you're referring to. The only function in
destroyer class is...

    template <typename T>
    void operator()(const T& operand) const

...but certainly all formal parameters (i.e., operand) are referrenced
in the function body.

What sort of warning are you getting from VC7?

> *Documentation:*
> tutorial.html:
> - a_printer and inst are not defined.
> - chacne -> chance
> - The next code snip, demonstrate -> The next code snip demonstrates
> sample.html:
> - What is the weight of a star or space ship? Totaling mass
> would make more sense to me. :-)
> - In the "bad" space example, total_weight's functions should
> return int.
> - Switching between using and not using the static_visitor
> base class was confusing to me. (I didn't notice it at first
> and was confused as to why result_type was defined sometimes
> but not others.)
> reference.html:
> - "All members of variant satisfy the strong guarantee of
> exception-safety, unless otherwise specified." This is
> inharmonious with the unqualified claim in the into "Strong
> exception-safety guarantee for all operations."
> - If check() is true - operator() -> If check() is true,
> operator() [otherwise, it looks like subtraction].

All noted. Thanks for feedback.

> - It would be helpful to document the space usage of the
> current implementation. Something like "pointer + 2 *
> sizeof(largest type) + padding" would be fine (assuming I
> have deduced correctly).

Noted. This seems like a good idea to me, at least as a non-normative

> *Misc.:*
> The copyright notice doesn't make clear that the copyright
> notice need appear only in source code copies.

I'm not sure what you mean. Could you please explain?

Thanks for your review and your comments,

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