RE: [boost] Formal review or Variant Library (Ed B.)

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

> *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
    {
        operand.~T();
    }

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

> *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,
Eric