Boost logo

Boost :

Subject: Re: [boost] [yap] Review part 1: documentation
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-11 20:26:09

On Sat, Feb 10, 2018 at 6:01 PM, Steven Watanabe via Boost <
boost_at_[hidden]> wrote:



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.

> - "...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.



> - 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.



- "--"
> 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/


> tutorial/evaluating_expressions.html:
> 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:

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.

> - "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/


> 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.

> - "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<unique_ptr>
> 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<unique_ptr> 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.

> examples/map_list_of:
> - map.emplace(
> Key{std::forward<Key2 &&>(key)},
> Value{std::forward<Value2 &&>(value)}
> );
> Why create extra temporaries here?

 Stupidity? Sleepiness? I just don't know. A very existential question.
Regardless, this is now changed.

> - return;
> 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 <typename Key, typename Value, typename Allocator>
    operator std::map<Key, Value, Allocator> () const
        std::map<Key, Value, Allocator> retval;
        map_list_of_transform<Key, Value, Allocator> transform{retval};
        boost::yap::transform(*this, transform);
        return retval;

> - "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


Boost list run by bdawes at, gregod at, cpdaniel at, john at