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/tuto... shows static_visitor being used with no arguments and yet
http://www.boost.org/regression-logs/cs-win32_metacomm/doc/html/boost/static...
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