On Sat, Feb 10, 2018 at 6:01 PM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
AMDG
[snip] All good notes; done. If you want to track the changes, I've created a boost_review branch where all the during-review changes will be done. tutorial/expressions.html:
- "...any of the functions in Boost.YAP that takes" s/takes/take/
That's not wrong; "takes" needs to agree with "any", not "functions".
- "And if we evaluate it using..." And don't begin a sentence with a conjunction.
:) Done.
- "An expression containing an expression<> subexpression and subexpressions instantiated from N other ExpressionTemplates can be passed as an argument to any of the Expression-accepting Boost.YAP functions." This sentence is hard to read because it uses "expression" too many times in too many different ways.
I've re-written it to hopefully make it clearer. Please let me know if it's still too hard to parse. [snip] Done. tutorial/operators.html:
- BOOST_YAP_USER_BINARY_OPERATOR_MEMBER(plus, ::lazy_vector_expr) I feel like it should be possible to avoid needing to specify the ExpressionTemplate. It should be possible to deduce the return type from `this`. Also, for binary operators, I'd really prefer to have a single macro that defines both expr + x and x + expr, as defining both is usually what we want for symmetric operators.
Agreed. There is a bug for this on GitHub already; I plan to add this after the review.
tutorial/explicit_transformations.html:
- "result is created by visiting each node in expr, in top-down, breadth-first order" Is it really breadth-first? I would find that quite surprising as normal recursion creates a depth-first traversal.
No, you're right. That's wrong on its face, but more broadly it's badly described. What I was trying to describe was the short-circuiting nature of the matching. If a visited subexpression is handled by the transform, the transform does not recurse from that point, unless you write that recursion into the transform. But that's not depth-first, of course. [snip] Done. - "--"
An mdash is \u2014 (or you can use UTF-8 for the qbk source, I think).
Thanks, I didn't know I could use Unicode characters in the qbk source in any form. "\u2104" works int the qbk source, btw.
- "evaluate() Is wrong for this task" s/Is/is/
Done.
tutorial/evaluating_expressions.html:
- I don't think that BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE is a good idea. Users can already define their own expression template types if the default behavior isn't good enough and your documentation encourages this anyway. A macro like this is just asking for accidental incompatibility.
I agree. This and the implicit transforms-during-eval are on the chopping block as far as I'm concerned. The main reason they have not already been cut is that I was waiting to see if reviewers really needed them to stay.
tutorial/operator_macros.html:
- What about BOOST_YAP_USER_ANY_UDT_BINARY_OPERATOR?
Do you mean "Why doesn't it exist?" If so, I don't know what it would do. BOOST_YAP_USER_UDT_ANY_BINARY_OPERATOR already accepts a UDT on either the right or left side, and any non-Expression type on the other. tutorial/how_expression_operands_are_treated.html:
- "This implies that an rvalue remain treatable as a..." s/remain/remains/. Also this sentence is somewhat awkward.
That's the right conjugation for what I was trying to say, though I agree about the awkwardness. I changed that fragment to "This implies that an rvalue be treated as if it were a temporary".
- T & - > PARTIAL_DECAY(T) & This can't possibly be right. If you decay a reference to a function or array into a pointer, then you need to treat the pointer as an rvalue.
Thanks! Fortunately, the code doesn't do what I wrote in that table.
tutorial/customization_points.html:
- what do you need evaluate_as for? I'm having a hard time thinking of uses that wouldn't be better handled as an explicit transform (Or perhaps just wrapping the expression in an implicit_cast_expr). The same goes for transform_expression. Having more knobs and dials just makes the API confusing. Also it looks to me like transform_expression will cause any placeholders to get dropped on the floor, as transform_expression doesn't take the extra arguments that would be used to replace placeholders.
- transform_expression is the odd man out in terms of naming. I think it should be eval_something. You're already using the term transform as something else.
Yes, I agree with all of this, for the reasons already stated.
tutorial/transform_matching.html:
- The negate example: struct xform { // This transform negates terminals. auto operator() (boost::yap::terminal_tag, double x) { return -x; }
// This transform removes negations. auto operator() (boost::yap::negate_tag, double x) { return x; } } seems somewhat confused. The behavior I would expect from this transform is that it can match expressions of the form `d` or `-d` and will return -d or d respectively.
This gets right at the point of the example. It's maybe a counterintuitive way to evaluate expressions, but the alternative is worse. The alternative is that terminal transforms are not auto-applied before a terminal is unwrapped and passed as a value to a tag-transform call. If that were how the library worked, there would be no way to write a tag-transform function that *did* apply the transform, even explicitly, because at that point the terminal is no longer visible. You are just passed a value (like "double x") above. Perhaps I should add something like what I just wrote here to the docs as well.
This implies that the correct behavior is not to apply the terminal_tag transform before accessing negate_tag. The later statement: // Only applies the negate_tag transform; prints -9. makes no sense as the negate_tag transform alone will give 9.
Thanks. Done.
- "The TagTransform is preferred, except in the case of a call expression, in which case the ExpressionTransform is preferred" Why is call treated specially? From an outsider's POV it just seems weird. At the very least it deserves an explanation in the rationale.
Frankly, because I'm not smart enough to fix this. I tried *a lot* to do so. When and if Paul ever puts Fit into a Boost release, I'm going to use that as the fix.
- "The above only applies when you have a ExpressionTransform" s/a/an/
Done.
examples/hello_world.html:
- I have no idea what you're talking about regarding eager evaluation in proto. The corresponding Proto example also uses an explicit evaluate.
I don't have any idea what I'm talking about there either. I went back and looked at all the Proto examples. and I can't figure out what it was I was looking at before that made me think that. I've removed the mention of Proto altogether.
examples/hello_world_redux.html:
- "That's better!" What's better than what?
This example is better that the previous example. The comment is tongue-in-cheek, though, as the rest indicates. examples/calc1.html:
- "Here we can first see how much C++14-and-later language features help the end user -- the Proto version is much, much longer." I disagree with this assertion. The real reason that this is shorter than the Proto version is that you've baked placeholder substitution into evaluate, which I think is a mistake--are you building a lambda library or are you building a generic ET library?
evaluate() hardly qualifies as a lambda library! However, the point remains the same -- making yap::evaluate() is trivial with variadic templates, and would be necessarily limited in a C++98 library. Making such a trivial function part of the Yap library instead of asking the user to reinvent that same wheel in every Yap terminal use case seems like a feature to me, not a bug.
examples/calc2.html:
- This really misses the point of proto's calc2 example. An equivalent example with YAP would be to define a new ExpressionTemplate with an overloaded call operator.
I don't think this example missed the point. In my calc2a example, I show the same functionality, just using lambdas to give the expressions names. I like this better for stylistic reasons. The calc2b version is more directly like the Proto version, except that here again it is possible to provide a single *very simple* Yap function that replaces all the functionality in the Proto calc2 example, yap::make_expression_function(). This also seems to me like a good thing.
examples/calc3.html:
- The way you're using get_arity is rather wacky as you know the arity statically anyway. The original Proto example makes more sense.
Please indicate which of the statements in Proto calc3's main() does *not* contain a statically known arity. Again, I think the only transformation that I made which lead to your comment was to give the expressions names by lambda-wrapping them. I prefer this style.
examples/lazy_vector.html:
- "This move is something of a hack." I'm not sure that I would consider it a hack. The general idea is that in evaluate(transform(expr)), any values held by expr should get moved into the result of transform so that they can be passed to evaluate.
You're right, of course. I only meant that it's a little wonky to std::move() and int to force Yap to make a copy.
- "We're able to write this more simply than the equivalent Proto code; since YAP is always lazy, we can just use the expressions below directly." Actually the same code works for proto as well. I suspect that BOOST_PROTO_AUTO was just used for symmetry with expr1.
True enough. Comment removed.
examples/vector.html:
- return boost::yap::make_terminal(std::move(vec[n])); Move is probably wrong as you're working with a reference to begin with. (Of course, it doesn't really matter for double, but imagine that you have a vector
instead.)
The move is required to force a copy; just because the reference is valid
now doesn't mean it won't dangle later. But in the case of a
vector
examples/mixed.html:
- I think you'd be better off making sin_tag a regular function object like in the proto example.
Could you explain? It isn't immediately obvious to me what you mean.
examples/map_list_of:
- map.emplace( Key{std::forward
(key)}, Value{std::forward (value)} ); Why create extra temporaries here?
Stupidity? Sleepiness? I just don't know. A very existential question. Regardless, this is now changed.
- return transform.map; This doesn't allow NRVO.
Thanks! I've changed the transform to operate on a reference to the return
type, and changed its use to this:
template
- "Assertion that left and right are compatible types" I don't think that is_same is the correct test. What you want to guarantee is that the return types are the same on both sides.
Yes, that's right. I've stricken the static_assert(), as it's not strictly necessary for the example anyway.
manual/object_code.html:
- "The object code generated by modern template metaprogramming can be quite good, in large part due to the drastic reduction in the amount of metaprogramming necessary to determine the result type" I'm not sure I see the connection here. Type-based metaprogramming generates no object code to begin with.
This is my intuition about part of what confuses the inliner -- I have seen the inliner "get confused" (my characterization) and produce worse code in these type-based metaprogramming-heavy situations, and then produce more inlined calls when the metaprogramming is removed from the same code. I could certainly be wrong! I'll strike that paragraph. Thanks for the very thorough review, Steven! You caught a lot. I'm looking forward to the second part, since I'm sure you'll catch a lot there too. Zach