Boost logo

Boost :

From: David Abrahams (dave_at_[hidden])
Date: 2006-11-19 22:21:56

Hi Eric,

Thanks for getting back to me on this.

> 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:
>> *
>> shows static_visitor being used with no arguments and yet
>> 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.

That one.

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

That's not enough reason to use derivation. Deriving from
std::iterator<...> is a waste of time by the same token. If you just
explain the nested result_type once, all the mystery about this when
reading the docs goes away.

That one.

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

No. It doesn't do that AFAICT. AFAICT it's just a simple interface

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

That one.

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

So fix that implication in the docs if it's wrong.

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

What's the minimal footprint? essentially max(sizeof(int),

Instead of starting with a long introduction about how C++ syntax
doesn't allow what you're trying to do, it would be most useful to
explain that last part much earlier.

  "recursive_variant_ is replaced by the library with a pointer to a
  dynamically allocated instance of the same variant type being

or something(?) Given that mental model, personally I'd be inclined to
replace std::pair with the dynamically-allocated thingy:

  class v :
    boost::variant<int, boost::recursive_wrapper<std::pair<v,v> > >

It would also be useful if the library docs could show how to make the
above effective, i.e., what ctors do I need?

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

So fix that in the docs.

> The intended point is that a unary visitor is not usable as a binary
> visitor

...unless it is.

> but perhaps it does not need to be stated.

Probably not. It's usually better not to say anything than to say
something you don't actually mean to say.

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

OK. I probably should have added that I think you could choose a
better name. An expression xxx_ptr(...) should always yield some kind
of pointer. Principle of least surprise, and all that.

Dave Abrahams
Boost Consulting

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