Re: [boost] [yap] review part 3: tests + misc + summary

Subject: Re: [boost] [yap] review part 3: tests + misc + summary
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-02-19 17:13:28

AMDG

I vote to *ACCEPT* YAP into Boost.

Critical issues (I believe that Zach has already addressed
most of these):
- The eval_xxx customization points.
- Handling of terminals for tag transforms.
- BOOST_YAP_CONVERSION_OPERATOR_TEMPLATE.
- Short-circuiting in evaluate.
- A single macro for symmetric binary operators.

TESTS:

- External dependencies are strongly discouraged. (i.e. gtest).
  Especially do not include a copy of external components.

- Boost.Build is required.

compile_test_main.cpp:

- This is basically unnecessary, as the compile tests
  don't need to be linked.

copy_only_types.cpp:

- This test is not useful, which is why it's #if'ed out.
  There is never a reason to create a type that can be
  copied from an lvalue but not an rvalue.  (unless a copy
  isn't really a copy, in which case YAP can't handle either
  one anyway.)

deref.cpp:

- the test cases in this file only show pass/fail, they
  don't indicate the actual type returned by deref, which
  will make debugging failures harder.  I typically use
  BOOST_MPL_ASSERT((is_same<X, Y>));  specifically for
  this reason (even when static_assert is available).

- Actually, in general, these tests need to check both
  the type and the value.  Checking just the type is
  insufficient.

user_expression_transform_3.cpp:

- It took me a while to find these tests for transform,
  because I (incorrectly) assumed that user_expression_tranform*
  were all tests for the transform_expression customization
  point after I looked at the first two.

vector_alloc_test.cpp:

- Your replacement of operator new is technically
  illegal, because it can return a null pointer.

- I don't think the standard guarantees that vector makes
  any specific number of allocations.  The correct
  way is to set allocations = 0 after initializing
  the vectors and then verify that no further allocations
  happen.  Alternately, make operator new throw a bad_alloc
  during the region where you expect no allocations.
  As a bonus, it'll be easier to find the cause of
  unexpected allocations, when they do happen.

You need some compile-fail tests:
- invalid arguments to accessors, such as left, right, and get.
- possibly switching the arguments of transform.  (I find
  it mildly disturbing that this currently compiles, as
  transform is one function for which I may not always remember
  the correct argument order.)

gcov says the following are not executed by the tests
(line numbers as of master):

algorithm.hpp:
  130 (value_expr_impl),
  232, 246 (get_impl),
  300 (get_c),
  408 (callable),
  427 (argument)
  701 (most of op_string)

expression.hpp:
  54, 62 (value),
  69, 77 (left),
  84, 92 (right),
  most unary operators (except negate)
  most binary operators (except multiplies and plus)
  247 (call),
  310 (value),
  428-420 (pre_dec, post_inc, post_dec)
  most binary operators (except multiplies, plus, and minus)

operators.hpp:
  many unary and binary operators.

print.hpp
  42 (print_value) - Isn't this overload for
      placeholders, rather than hana::llong?
  50,52,54 (print_type),
  87,89 (print_impl)

detail/default_eval.hpp:

  20 (eval_placeholder)
  many unary and binary operators.

EXAMPLES:

doc/user_macro_snippets.cpp:268
- This should be is_vector not is_string.

doc/other_snippets.cpp:90
    std::cout << evaluate(expr) << "\n"; // Prints "Yay."  Wierd!
  s/Wierd/Weird/

General Notes:

- There are three primary modes for processing an
  expression tree, which I will describe as:
  1. transform: Takes a tree and produces another tree.
  2. evaluate: Takes a tree and evaluates it in some way.
  3. for_each: Processes each node and returns nothing.
  yap::transform can handle all three as long as you explicitly
  handle all nodes.  The default behavior is (1), which
  makes (2) and (3) somewhat inconvenient.  Your solution
  in the examples seems to be transform/evaluate for (2)
  and returning a dummy result for (3).  Unfortunately,
  transform/evaluate doesn't work well if evaluation
  involves any special constructs that affect control-flow.
  Just try to handle something like this:
    let(_a=2) [ _a + 1 ] // evaluates to 3
  Returning a dummy result is a bit annoying, but shouldn't
  cause any real problems, as long as terminals are captured
  by reference in the result.  All in all, I'd like to have
  some way to change the default behavior of transform, or
  perhaps have separate functions with different default behavior.
  Just an idea: switch the default behavior based on the
  result of transforming the subexpressions:
  Expression/non-Expression/void.
  This has the side effect that you must explicitly wrap
  terminals when returning, but I think that's actually a
  good thing, as a transform that returns unwrapped terminals,
  expecting them to be wrapped by the caller, may have
  inconsistent behaviour.
  In addition, there is another possible mode that has
  better type safety which I will call:
  4. manual: No default behavior.  If a node is not handled
     explicitly, it is a hard compile error.

- Combining transforms isn't exactly easy, because of
  the way transforms recurse.  For example, if I have
  two transforms that process disjoint sets of nodes,
  I can't really turn them into a single transform.

- How useful is it to separate the Expression concept
  from the ExpressionTemplate concept?  For example,
  transform requires an ExpressionTemplate for nodes
  that are not handled explicitly, but that isn't very
  clear from the documentation, which just says that
  it takes an Expression.

- You say that it's fine to mix and match expressions that
  are instantiated from different templates.  Why would
  I want to do this?  The first thing that comes to mind
  is combining two independent libraries that both use YAP,
  but I suspect that that won't work out very well.
  It seems a bit too easy for a transform to inadvertently
  recurse into an unrelated expression tree in this case.

- The value of a terminal cannot be another expression.
  Is this a necessary restriction?  (This is related to
  mixing different libraries using YAP, as the most
  common form is for library A to treat the expressions
  from library B as terminals.  Since a terminal can't
  hold an Expression, we would need to somehow un-YAP
  them first.)

- BOOST_YAP_BINARY_OPERATOR_MEMBER(assign) needs
  special handling because of the copy assignment
  operator.

- Unwrapping terminals redux:
  Unwrapping terminals is convenient when you have
  terminals of the form:
  struct xxx_tag {};
  auto xxx = make_terminal(xxx_tag{});
  AND you are explicitly matching this terminal with
  auto operator()(call_tag, xxx_tag, T...) or the like.
  I expect that matching other terminals like double
  or int is somewhat more rare in real code.  If you
  are matching any subexpression, then unwrapping
  terminals is usually wrong.  Even in the first case,
  where unwrapping terminals is sometimes convenient,
  it prevents usage like:
  auto operator()(call_tag, decay_t<decltype(xxx)> const &);
  (Which is how I might write a matcher if xxx is not defined
  by me and xxx_tag is not a documented public symbol).

In Christ,
Steven Watanabe