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. * 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. * 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. * 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. * 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. * 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? -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
I'm just looking through the variant library docs in detail and had the following commments:
Ah, if only boost::variant was actively maintained... I had several issues raised in http://lists.boost.org/Archives/boost/2006/06/105801.php and some time later I encountered another issue - I think the default constructor for variant is a bad idea. The thought came to me after I made a mistake of writing typedef variant< optional<X>, optional<Y> > SomeType; when I really meant was typedef optional< variant<X, Y> > SomeType; later in my code, thinking I had the right typedef, I constructed an instance of SomeType using its default constructor, assuming I had an empty optional<variant> in my hands, while I actually had a variant with an empty optional<X>. Unfortunately, it compiled, and that mistake was only found in run-time, with the help of the QA department. If variant didn't have a default constructor, my mistake wouldn't have compiled. IMO, this is also true from a more theoretical point of view - there is no reason to favor one of the variant types (the first one) over the others. Those types are basically equivalent and symmetric. If the user wishes to add a default constructor himself, he can derive from the variant, and add such a constructor, favoring the type of his choice. Or maybe the best solution would be to add a template parameter to boost::variant to indicate which of the types to use for the default constructor, with an option (the default one) to not supply a default constructor at all. Of course I'm just saying all this here for Google's sake. Nothing real would probably be done. Sad... Yuval
Yuval Ronen <ronen_yuval@yahoo.com> writes:
David Abrahams wrote:
I'm just looking through the variant library docs in detail and had the following commments:
Ah, if only boost::variant was actively maintained...
Would you like to take over maintenance of boost::variant? It shouldn't be a big job; the interface is fairly stable. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
Yuval Ronen <ronen_yuval@yahoo.com> writes:
David Abrahams wrote:
I'm just looking through the variant library docs in detail and had the following commments: Ah, if only boost::variant was actively maintained...
Would you like to take over maintenance of boost::variant? It shouldn't be a big job; the interface is fairly stable.
As incredibly tempting as it sounds (really, it does!), I'm afraid I won't be able to truly provide support and maintenance for the library. I'll probably use this mandate to get several things I want to change, but then I won't have enough time to properly test it, document it and port it to all platforms. At the end of the day, if I'm not fully committed to invest the time needed, which I'm afraid I'm not, I'll do more harm than good. Of course I'm not against having write access to the Boost CVS, but it's unlikely I'll use it for the above mentioned reason. But thanks a lot for the offer!
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/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.
* 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/v... http://boost-sandbox.cvs.sourceforge.net/boost-sandbox/boost-sandbox/boost/t...
* 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
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
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/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.
* 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/v... http://boost-sandbox.cvs.sourceforge.net/boost-sandbox/boost-sandbox/boost/t...
* 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
participants (3)
-
David Abrahams -
Eric Friedman -
Yuval Ronen