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

From: Ed Brey (brey_at_[hidden])
Date: 2003-02-17 10:55:15

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:

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

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.

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.

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.

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.

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

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

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

*Misc.:*

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