From: Eric Niebler
Steven Watanabe wrote:
users_guide\expression_construction\tags_and_meta_functions.html tag::posit: I don't like this name. It confused me at first since posit is a complete word which means something totally different. positive would be better. IMO.
My (screwy?) logic is as follows: unary minus is called "negate" because there is a std::negate function object that applies unary minus. And what is the opposite of the verb "negate"? It's "posit".
?? The New Collins Concise English Dictionary (1982) by my desk has: Negate: 1. to nullify; invalidate. 2. to contradict Posit: 1. to assume or put forward as fact or the factual basis for an argument. 2. to put in position. I don't really see how you can say that "to posit" is the opposite of "to negate".
ternary ?: You can't overload that operator!?
So? That doesn't mean I can't have a tag for it and process it in default_context with ?: as if it had been overloaded. Proto provides an if_else() ternary function, which behaves as the ?: operator.
nary function call n-ary function call?
operator(). Yes, should I hyphenate?
users_guide\expression_introspection\if_and_not.html: if_< is_same< _arg, char const * >() > Why ()? Why a transform? Is this boost::is_same?
Yes, this is boost::is_same. What else would I use there aside from a transform?
users_guide\expression_introspection\defining_dsel_grammars.html: We can use matches<> and BOOST_MPL_ASSERT() to issue readable compile-time errors for invalid expressions, as below: I'm only partially convinced. The error message will be short, sure, but if the expression is complicated, it may not be enough information, since this a very coarse grained error message.
It would be nice to be able to tell people *why* their pattern doesn't match the grammar. It's a hard problem and I haven't figured it out yet.
It is probably better than the alternative, which would be to generate a cryptic error and megabytes of spew. For those willing to wade through the spew and examine the library source, it /might/ provide more context to understand the error. Those people are few, IMO.
users_guide\expression_transformation.html: A grammar decorated with transforms is a function object that takes three parameters: expr -- the Proto expression to transform state -- the initial state of the transformation visitor -- any optional mutable state information What type should "state" and "visitor" be? Why are they two different arguments?
Whatever you want. There are two different parameters because some transforms (e.g., fold) will update the state parameter automatically. Sometimes you want to pass around a mutable blob that only gets modified by you, not by fold. That's when you use a visitor.
The Haskell "Scrap Your Boilerplate" technique (aka, The Mother of All Traversals) has similar parameters. The state is called an "accumulation" and the visitor is called the "context". I designed this part of Proto before I knew about the Haskell work. I think it's interesting that we converged on the same functional specification.
Do transforms work if the grammar has transforms at any level? Or do I need a transform at the top level?
All grammar elements have a default transform. So if the default on the top level is acceptable to you, you don't need to specify it.
users_guide\expression_transformation\canned_transforms\if.html: The result of applying the first transform should be a compile-time Boolean. If it is true... Meaning it should be an MPL Integral Constant Wrapper? If non-zero?
Yes.
users_guide\expression_transformation\canned_transforms\make.html This uses proto:::make<> under the covers to evaluate the transform.) s/:::/::/
Thanks.
users_guide\expression_transformation\canned_transforms\when.html that it, when<> also behaves like a transform. s/it/is/
Thanks.
users_guide\expression_transformation\canned_transforms\fold_tree.html:
fusion::cons<FoldTreeToList, _state>(FoldTreeToList, _state) I'm getting confused. Let me see... This is a transform that applies FoldTreeToList to its argument and uses the result to construct a fusion::cons with the current state. Question. Why reverse_fold_tree? would reverse_fold work as well because it would first evaluate the right side of the comma, creating a fusion::cons and then take the left side and push it onto the front?
I think you'd end up with a cons<cons<>, cons<> >, which is not what you want. Try it and see!
users_guide\expression_transformation\is_callable.html: ...a problem for a type like std::vector<_arg(_arg1)>(), which is a valid transform... Can't you substitute /first/ and /then/ check is_callable?
I could do that, yes. And I seem to recall having a reason for not doing that.
... time passes ...
Either the reason escapes me, or else my reasoning was wrong. I'll investigate this.
users_guide\expression_extension\extends.html: What do I do if I want a statically initialized terminal that has a custom domain? Inhetitance from extends<> makes my type a non-aggregate...
Right, there is a BOOST_PROTO_EXTENDS() macro for this, but it's already been noted that is isn't documented.
It looks like the comments in users_guide\examples\mixed.html in DereferenceCtx and IncrementCtx were copied from vector.html and not updated. Also, VectorOps should be MixedOps?
Thanks.
boost\proto\args\hpp.html Why is args0 unary!?
I'm sorry, where are you looking? args0<> does have one element in it: the type of the terminal. It has 0 child expressions, though, so its arity is really zero.
boost\proto\context\callable_eval.html: context(Expr::proto_tag(), arg_c\<0\>(expr), arg_c\<1\>(expr), ...) s/\//
Thanks.
Why does callable_eval have no members listed?
Because the primary template is indeed empty. Only the specializations have members.
boost\proto\context\callable_context.html: Is it just more boostbook weirdness that the Synopsis has an empty eval<>?
Yes, eval<> has a base that isn't showing up. BoostBook bug. Crud.
boost\proto\context\callable\hpp.html: Do you really need to show all the specializations of callable_eval?
Probably not, but getting something else would involve waging a costly war with our documentation toolchain.
boost\proto\context\null\hpp.html ditto.
Right.
boost\proto\display_expr_id535716.html: The one argument form writes to std::cout, right?
Yes, noted.
boost\proto\domainns_\domain.html: boost::proto::domainns_::domain Is domainns_ a private namespace to block ADL? If so, it should not appear in the documentation...
You're right. More battles with the documentation toolchain. :-(
boost\proto\functional\eval.html: A general comment about eval: It should be noted that the context is *not* copied, and so it is safe for it to have mutable state. (Since this is not usual for function objects which is what callable_context implements)
True.
boost\proto\expr\hpp.html: I don't think that you should need to show all the specializations of expr in the documentation. exprns_ is a private namespace? It shouldn't appear in the documentation.
I guess. :-/ The fact that these types live in special namespaces is a part of the interface, in a way, in that it avoid ADL problems. I'm of two minds about this.
boost\proto\exprns_\expr_Tag,Args,0__id530744.html: operator= is specified as returning the same type!? Is this a doxygen or boostbook problem?
Doxygen or BoostBook, I don't know.
static expr::make... What are the overloads that take a size_t template parameter for?
So that you can initialize a terminal<int[4]>::type -- that is, you can have a terminal that holds an array by value.
boost\proto\fusion\hpp.html What is the rational for only bringing flatten into namespace proto.
There's no sense having proto::pop_front() and proto::reverse(). They would do the same thing as fusion::pop_front() and fusion::reverse(). I wouldn't even need these if Fusion provided function object equivalents of its free functions.
boost\proto\is_extension.html: What does this do?
Another piece that fell through the cracks. It lets you non-intrusively adapt a type to be a Proto terminal, as long as you bring the proto::exops namespace into scope (which is also undocumented, argh!).
boost\proto\operators\hpp.html: Why does if_else specify the return type while all the others say /unspecified/
An implementation artifact. The binary operators do not have their return type expressed in terms of make_expr<> because it would incur unnecessary template instantiations. That's tractable with only 2 argumemts. With 3, the overload set and return type calculation became too tedious to write out, so I just used make_expr<> and required the parameters to be const&. Not ideal, but also not a big deal, IMO.
boost\proto\proto_fwd\hpp.html: this is not pretty.
It's a horrible mess. Doxygen doesn't emit documentation for forward declarations, but it does for typedefs. Not sure what to do about it. :-/
boost\proto\transform\fold\hpp.html: Why does this say that the it contains fold_tree, but fold_tree doesn't appear in the listing.
I can't imagine why. The comment from fold_tree.hpp seems to have mysteriously migrated there. Ah! In fold_tree.hpp, I have mistakenly labeled it with \file fold.hpp. Fixed.
boost\proto\transform\reverse_fold.html: This says that reverse_fold simply inherits from fold!?
[sigh] I know. reverse_fold really inherits from fold<call<_reverse(Sequence)>, State0, Fun>. I tried everything to get it to show up correctly, but lost that battle with Doxygen. Really, sometimes I wonder if Doxygen is more trouble than it's worth. At other times, I'm sure it is.
boost\proto\transform\fold_tree.html: Is there some way to do a fold tree that recurses over some set of tags rather than a single fixed tag?
No, but it wouldn't be difficult to define. fold_tree's implementation is dead simple.
boost\proto\transform\make.html: Function pointer types How are function pointer types interpreted as transforms? Oh. OK. They are treated the same as functions.
Source:
args.hpp: No comments. (Besides what I said about arg0 earlier)
context.hpp: no comments.
debug.hpp: why doesn't this use detail/prefix.hpp and suffix.hpp
It probably should.
This should include detail/dont_care.hpp
Right.
Why does the display_expr take the depth first? I would expect that users would want to override the default output stream more often then the starting depth.
You might be right. I have no strong feelings.
deep_copy.hpp: Ought to #include <boost/mpl/if.hpp>
Right.
The global function object proto::deep_copy can cause the usual subtle ODR violation for objects in headers with internal linkage.
Right. It should be a function, anyway, so ADL can find it.
I don't think that this header needs to #include <boost/proto/generate.hpp>
domain.hpp: This should #include <boost/ref.hpp> and does not need to
Right.
#include <boost/proto/generate.hpp>
Actually it needs this one because one of domain<>'s template parameters defaults to default_generator, defined in generate.hpp. People should be able to use domain<> without having to separately #include generate.hpp.
eval.hpp: No comments. (Yeah!)
Whew.
expr.hpp: Why is operator= only overloaded for non-const expr's when they are terminals? Same thing for operator[]
IMO, they're unlikely to ever be used and leaving them out measurably improves compile times. They would only conceivably be used in expression like (a+b)=x or (a+b)[x]. However, Proto's operator overloads return const rvalues, so a non-const operator= or operator[] on a non-terminal would never even be considered. Ditto for operator(), where leaving the const overloads out has a dramatic effect on compile times.
Doesn't use #include <boost/preprocessor/arithmetic/inc.hpp> nor #include <boost/preprocessor/facilities/intercept.hpp> You're not using #include <boost/utility/result_of.hpp>
Thanks.
extends.hpp: needs to #include <boost/preprocessor/repetition/enum_trailing_params.hpp> and <boost/preprocessor/repitition/repeat_from_to.hpp>. Does not need <boost/preprocessor/arithmetic/dec.hpp> or <boost/preprocessor/facilities/intercept.hpp>
Agreed, except for dec.hpp.
Should there be a BOOST_PROTO_EXTENDS_ALL macro, so that I don't have to write out all four macros? At the very least, you need to note that BOOST_PROTO_EXTENDS does not overload [], (), and =, since it differs from boost::proto_extends in this respect.
Right, I might rename BOOST_PROTO_EXTENDS to BOOST_PROTO_BASIC_EXTENDS, and have BOOST_PROTO_EXTENDS mean the same thing as extends<>. And document it. :-/
fusion.hpp: Needs to #include <boost/mpl/if.hpp>
Gotcha.
Why do you put BOOST_PROTO_CALLABLE() in the definitions of functional::flatten/pop_front/reverse and then specialize is_callable too?
You're right, I don't have to. The is_callable specialization shortcuts a few template instantiations, tho. And BOOST_PROTO_CALLABLE() is so that any types derived from flatten/pop_front/reverse are also callable. (As if anybody would want to do that.)
The specializations of fusion::extension::is_view_impl use Iterator as the name of the template parameter of apply??
Typo.
Why does value_of_impl<proto::tag::proto_expr_iterator> have a specialization of apply for references? The primary template works, doesn't it?
Yup.
generate.hpp: What is the point of expr_traits? Doesn't expr expose this information directly? It is in namespace detail so it isn't intended to be specialized by users is it?
Correct. I have to be very careful here not to do anything which could instantiate the expression type here. There are places in the code where the return type of an expression's member function is expressed in terms of generate, which is expressed in terms of the expression type, etc... Loopy compile problems.
Is there a particular reason for not defining a generator as a polymorphic function object? Efficiency?
No good reason. I should change it.
The template template arguments for pod_generator are worrisome. Take a look at the example in extends.hpp for is_proto_expr. If I want to make a POD wrapper it seems that I need /two/ template arguments and thus cannot use pod_generator at all...
Well, in that example, my_terminal<> is a terminal wrapper, not a general expression wrapper, so you wouldn't pass it to pod_generator. I understand your concern, but I don't want to use something like an mpl placeholder expression here because of overhead. The generator protocol is so dead simple that is generate<> and pod_generate<> don't mean your special needs, it takes ~2 minutes to bang together a custom generator that does.
make_expr.hpp: You might note the format of DATA in the macros. I'm finding it difficult to work out what they are doing.
What's your objection?
BOOST_PROTO_AT_TYPE(...) why add_const?
If fusion::value_at<> returns a reference, add_const is a no-op. Otherwise, it lets me pass rvalues. IIRC.
The implementation of BOOST_PROTO_VARARG_TEMPLATE_YES_ converting a seq to a tuple to a list is clumsy. Why isn't it implemented using BOOST_PP_SEQ_FOR_EACH_I_R? Same thing goes for BOOST_PROTO_TEMPLATE_PARAMS_YES_ What is BOOST_PROTO_SEQ_PUSH_FRONT for? Oh. I get it. This is already being instantiated inside a BOOST_PP_SEQ_FOR_EACH_I... I'm thinking that these macros belong in their own file.
It's all very complicated. I took me days to write those stupid macros. Please don't make me look at them again -- they make my eyes bleed. The best that can be said about them is that they work. I confess I'm a terrible PP metaprogrammer.
matches.hpp Should include <boost/type_traits/is_array.hpp>
Good catch. How do you do that?
I don't quite get vararg_matches. The specialization for Zero = false looks fishy. If I understand correctly, you are explicitly forcing the size to /include/ the vararg argument. In fact, I'm not sure that I like the fact that you are forcibly overriding the size of the arg sequence at the expr level at all. In addition, the argument "From" to vararg_matches_impl will skip over the argument that corresponds to the vararg. What am I missing? Oh. I see, vararg_matches_impl doesn't take a half-open range... Is there a noticeable performance improvement vs. doing this more naturally? Ok it looks right, but it took quite a bit of work to demonstrate.
I have sunk weeks into the implementation of proto::matches<> to make it as fast as possible. I play some dirty pool in there, I know. I think it's all kosher, tho.
I think that you are testing the element that is in the same position as the vararg<> in both matches<> and vararg_matches<>, though?
Really, where? I don't see that. Keep in mind that template instantiations are memoized by the compiler. Asking for the same instantiation twice is "free".
Why is matches_ specialized to return false for a terminal tag and proto::_?
Huh! That doesn't look right, does it? Could be a relic of the time when terminals were degenerate unary expressions.
operators.hpp: no comments.
proto.hpp: no comments.
proto_fwd.hpp: Why do you hard code arg0, arg1, ... rather than using the preprocessor? I know it's easy but it's still not configurable.
That's pretty dumb of me. I'll fix it.
proto_typeof.hpp: no comments.
ref.hpp: why does unref leave array references as references, but does not treat plain arrays differently from other values?
You're right, I'm being a bit schizo about array types. The question is, does deep_copy cause arrays to be stored by value? I worry about immense amounts of data getting copied all over. But maybe the answer is yes, it probably should. A simple reference wrapper avoids the copy in that case. At the very least, I need to be consistent.
In other files you create an object of type functional::* for unref you duplicate the overloads at namespace scope. It would be best to stick to a single style consistantly, IMO.
Yes, they should all be free functions, so ADL can find them.
tags.hpp: no comments.
traits.hpp: I don't see what BOOST_MPL_AUX_LAMBDA_ARITY_PARAM in detail::callable is for.
It's for working around gcc bugs.
Why does as_expr not copy arrays? It differs from deep_copy in this?
More schizo array weirdness. Will fix.
All the operator definitions are the same thing over and over and over again. Why don't you use a macro? Oh. Except the documentation comments...
Yes, it's for Doxygen.
context/callable.hpp: no comments.
context/default.hpp no comments.
context/null.hpp no comments.
detail/as_lvalue.hpp The name uncv is misleading since it is only unconst.
It's a detail, but ok.
detail/dont_care.hpp no comments.
detail/funop.hpp: no comments.
detail/pop_front.hpp: no comments.
prefix.hpp: no comments.
reverse.hpp: no comments.
suffix.hpp: no comments.
transform/arg.hpp: why does transform::_ref have a leading _ unlike expr?
Because if it were just "ref" then, with gcc's strange name look-up implementation, it would be ambiguous with the boost::ref() function. Crazy and stupid, but not my fault.
transform/bind.hpp: no comments.
transform/fold.hpp: I don't think that detail::as_callable is a good name. It should be more like: bind_visitor_and_transform_for_fusion_fold
Ha! A bit verbose, perhaps...
transform/fold_tree.hpp: no comments.
transform/make.hpp: Why are there specializations of construct_ for expr both for any number of arguments and for each number of arguments?
Damned if I know. :-D I'll fix it.
The documentation says that If X is a template like Object2<Y0,Y1,...>, ... Otherwise, if X is a transform ... The tests actually happen in the opposite order. In other words, a templated transform is treated as a transform, not a template.
Great catch. Looks like a documentation bug, I think the code is doing the right thing.
transform/pass_through.hpp: no comments.
transform/when.hpp no comments.
General:
It would probably better to define a macro BOOST_PROTO_HAS_SEPARATE_FUSION rather than constantly saying #if BOOST_VERSION < 103500
Why is that better?
You obviously put a huge effort into this review. I'm deeply grateful for your contribution. Thank you.
-- Martin Bonner Senior Software Engineer/Team Leader PI SHURLOK LTD Telephone: +44 1223 441434 / 203894 (direct) Fax: +44 1223 203999 Email: martin.bonner@pi-shurlok.com www.pi-shurlok.com disclaimer