Boost logo

Boost :

Subject: Re: [boost] [yap] Review part 1: documentation
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-02-11 00:01:44


  I have not included the doxygen reference in this
as I'll get to that along with the code.


- missing .gitattributes


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


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


- "...any of the functions in Boost.YAP that takes"

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


- "kinds of expression"

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


- 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


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

- "If situations when you want to..."

- "--"
  An mdash is \u2014 (or you can use UTF-8 for the
  qbk source, I think).

- "evaluate() Is wrong for this task"


  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.




- "This implies that an rvalue remain treatable as a..."
  s/remain/remains/. Also this sentence is somewhat

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


- 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


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


- I have no idea what you're talking about regarding eager
  evaluation in proto. The corresponding Proto example
  also uses an explicit evaluate.


- "That's better!"
  What's better than what?


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


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


- The way you're using get_arity is rather wacky as
  you know the arity statically anyway. The original
  Proto example makes more sense.


- "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
  Actually the same code works for proto as well. I suspect that
  BOOST_PROTO_AUTO was just used for symmetry with expr1.


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


- I think you'd be better off making sin_tag a regular
  function object like in the proto example.


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

- return;
  This doesn't allow NRVO.


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


- "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, gregod at, cpdaniel at, john at