|
|
Boost : |
Subject: Re: [boost] [yap] Review part 1: documentation
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-02-11 00:01:44
AMDG
I have not included the doxygen reference in this
as I'll get to that along with the code.
General:
- missing .gitattributes
introduction.html:
- "...expression syntax that ... optimizes your expressions. "
The syntax doesn't optimize. It's the Eigen implementation
that optimizes.
- Please use the correct syntax for bulleted lists.
It's '*' not '-' in quickbook.
header_organization.html:
- I think it's better to put the actual name of the header
as the primary text since that's what I actually need to
know to #include it.
tutorial/expressions.html:
- "...any of the functions in Boost.YAP that takes"
s/takes/take/
- "And if we evaluate it using..."
And don't begin a sentence with a conjunction.
- "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.
- "there are lots of examples of how to do this in the Examples section"
A link on "Examples" would be nice.
tutorial/kinds_of_exceptions.html:
- "kinds of expression"
s/expression/expressions/
- "represents the leaf-node in an expression tree"
I think "a leaf-node" would be more correct as
an expression tree may have more than one leaf.
- "An if_else expression is analogous to the C++
ternary operator (?:), but without the common-type
conversion logic."
So does this mean that such conversions are
represented explicitly or are they simply not
allowed?
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.
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.
- "In other words, if returns any non-Boost.YAP argument..."
s/if/it/
- "If situations when you want to..."
s/If/In/
- "--"
An mdash is \u2014 (or you can use UTF-8 for the
qbk source, I think).
- "evaluate() Is wrong for this task"
s/Is/is/
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.
tutorial/operator_macros.html:
- What about BOOST_YAP_USER_ANY_UDT_BINARY_OPERATOR?
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.
- 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.
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.
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 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.
- "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.
- "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.
examples/hello_world_redux.html:
- "That's better!"
What's better than what?
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?
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.
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.
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.
- "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.
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.)
examples/mixed.html:
- I think you'd be better off making sin_tag a regular
function object like in the proto example.
examples/map_list_of:
- map.emplace(
Key{std::forward<Key2 &&>(key)},
Value{std::forward<Value2 &&>(value)}
);
Why create extra temporaries here?
- return transform.map;
This doesn't allow NRVO.
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.
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.
In Christ,
Steven Watanabe
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk