Re: [boost] [variant] late "review"

From: Eric Friedman (ebf_at_[hidden])
Date: 2006-11-18 23:47:42

Hi Dave,

I assume most of your comments are intended to suggest places where I 
could improve the documentation, but I will answer them directly in any 
case. If you could suggest which of these points should make it back 
into the documentation, I would appreciate your opinion.

David Abrahams wrote:
> I'm just looking through the variant library docs in detail and had
> the following commments:
> 
> * http://www.boost.org/regression-logs/cs-win32_metacomm/doc/html/variant/tutorial.html 
>   shows static_visitor being used with no arguments and yet
>   http://www.boost.org/regression-logs/cs-win32_metacomm/doc/html/boost/static_visitor.html
>   doesn't indicate any default for the first parameter.  Also, once
>   you start using an argument in the tutorial, it comes with no
>   explanation, which is confusing.

The default for the first parameter is void. Yes, I could probably 
explain this in the tutorial.

> * It's not obvious why we're deriving anything from static_visitor.
>   It seems as though it shouldn't be necessary and that it might not
>   be simplifying anything.

It provides the nested result_type, which is required for the 
StaticVisitor concept.

More specifically, I had intended to develop a Boost.Visitor library 
that would provide support for static and dynamic visitors as well as 
other constructs such as type switches. There is actually some rather 
old code in the sandbox:

http://boost-sandbox.cvs.sourceforge.net/boost-sandbox/boost-sandbox/boost/visitor/ 

http://boost-sandbox.cvs.sourceforge.net/boost-sandbox/boost-sandbox/boost/type_switch.hpp

> * I find the dual nature of apply_visitor confusing.  With two arguments it
>   is an imperative command: "apply this visitor."  With one argument
>   it is a wrapper generator or something like that: "make this visitor
>   into a function object that operates on variant<T0,...TN> instead
>   of on T0, ... TN directly."  [Actually, I'm not sure the latter
>   description is accurate and it wouldn't hurt to have it spelled out
>   in the docs.]  I think it would be better to use two names rather
>   than overloading one name in these two very different ways.

Would you prefer apply_visitor_delayed?

> * I have a very clear mental model for what a non-recursive variant is
>   doing, which gives me a clear of its costs and of where I'd use it:
>   it's like a supercharged union.  From reading your docs it's very
>   hard to form the same kind of mental model for a recursive variant.
>   It would help to be explicit about what the library generates for
>   you in the recursive case.
 >
> * The make_recursive_variant example is vague.  It should describe
>   what the result is equivalent to.  I presume it's something like:
> 
>        variant<int, ...
> 
>   ...uhh, interesting: I don't think you can express that example in
>   terms of just variant and recursive_wrapper.  However, the text
>   implies that make_recursive_variant is just a less-flexible way to
>   achieve a subset of the same goals.

Suppose we could write a recursive variant like this

   RV = boost::variant< int, std::pair<RV,RV> >

Of course this is not syntactically possible in C++, but pretend for a 
moment. The true problem, as the tutorial notes, is that this type would 
require an infinite amount of storage to allocate statically.

Using boost::recursive_wrapper solves this problem by allocating its 
contents dynamically. We could write

   RV = boost::variant<
      int,
      std::pair<
         boost::recursive_wrapper<RV>,
         boost::recursive_wrapper<RV>
         >
      >

Even if this syntax were possible, it would be inconvenient. The
short-hand alternative is to use make_recursive_variant:

   boost::make_recursive_variant<
      int,
      std::pair<
         boost::recursive_variant_,
         boost::recursive_variant_
         >
      >::type

Specifically, the pair's contents are dynamically-allocated.

> * binary visitation:
> 
>          "Notably this feature requires that binary visitors are
>          incompatible with the visitor objects discussed in the
>          tutorial above, as they must operate on two arguments."
> 
>   I don't get it.  I can make an object whose function call operator
>   overload set can operate on both 1 and 2 arguments.

Perhaps "incompatible" is not the best word choice. The intended point 
is that a unary visitor is not usable as a binary visitor, but perhaps 
it does not need to be stated.

> * It seems like visitor_ptr is a very specialized name for something
>   very general-purpose.  Doesn't bind(fp) (or some other boost
>   construct) do the same thing?

Not that I know of. In particular, a visitor must be able to accept all 
of the variant's bounded types. What boost::visitor_ptr does is to throw 
boost::bad_visit in the event that it is visited with another type.

Eric