|
|
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,
Yep
> but I will answer them directly in any
> case.
Thanks.
> If you could suggest which of these points should make it back
> into the documentation, I would appreciate your opinion.
OK
> 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.
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
transformation.
>> * 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),
sizeof(pair<void*,void*>))?
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
specified..."
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 www.boost-consulting.com
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk