AMDG On 02/11/2018 01:26 PM, Zach Laine via Boost wrote:
On Sat, Feb 10, 2018 at 6:01 PM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
[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".
I think "any" is also plural when used as an indefinite pronoun.
<snip>
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.
Oh, I see. I got confused by the fact that the table says "--" for the second operand type instead of saying the same as for the first operand type, so I jumped to the conclusion that the macro is asymmetric.
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.
I don't believe that that is worse than being unable to write a tag-transform that *doesn't* apply the transform. Nothing else in a transform implicitly recurses, so I think your choice here is inconsistent. In addition, it's not quite true that you can't apply it explicitly. I can think of at least three ways to do so: - re-wrap the argument in a terminal - call (*this)(terminal_tag(), d) - implement terminal evaluation in a separate function which can be used as needed.
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.
Okay, so it's an implementation problem rather than a deliberate interface choice made for some inscrutable reason. I can live with that.
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.
Ah. Having "That" appear a full paragraph before its antecedent makes it unintelligible.
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.
Positional placeholders are really only needed if you're doing lambda-like things. I don't think that the primary library interface should directly support domain-specific uses. I also don't think requiring those who actually need this to implement it themselves is an excessive burden, as a transform that does it should be <10 lines of code. If you really feel that it's necessary to provide this, just write the transform yourself and provide it along with the placeholders.
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.
The point of the example in Proto is how to add members to an expression type, not just how to make it callable. Wrapping it in another class is something quite different. Now, admittedly, Yap makes creating your own ExpressionTemplate types so easy, that this example is basically redundant...
In my calc2a example, I show the same functionality, just using lambdas to give the expressions names.
You can give the expressions names in Proto too: BOOST_PROTO_AUTO(expr_1_fn, _1 + 2.0);
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.
The arity is known in main, but is *not* known at the point where get_arity is actually called. i.e. in the various overloads of calculator_expression::operator()
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.
Sure, but when using the evaluate(transform()) idiom, you're guaranteed that the references returned by transform will not be left dangling.
But in the case of a vector
case, yes I would probably have done something different. 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.
struct sin_tag { template<class T> T operator()(const T& t) const { using std::sin; return sin(); } }; Then there's no need to use a yap-specific customization point (eval_call). In Christ, Steven Watanabe