Re: [boost] [yap] Review part 1: documentation

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:

> 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<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 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 <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;
    }

examples/future_group.html:
>
> - "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