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
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
