[boost] [yap] Review

Subject: [boost] [yap] Review
From: P F (pfultz2_at_[hidden])
Date: 2018-02-12 22:15:23

- Whether you believe the library should be accepted into Boost

I think it should be Accepted.

- Your name

Paul Fultz II

- What is your evaluation of the library's:

The design is very nice and simple to follow. I have found Proto to be a little hard for me to grep, but this seems fairly simple to follow.

I think the transform -> evalute workflow is great. Also, with transforms being just function objects, it makes transformations very composable.

I think this library is very useful. I would love to use this for some projects at work, but the dependency on Hana prevents that(I need to support gcc 5 and MSVC). Looking at the codebase, it could easily be replaced with Boost.Fit(once its included in the release) in the future.

Of course, I dont expect a reimplementation without Hana as a condition for acceptance. However, I think Hana at least can be removed from the "interface". The expression elements could be expressed as a `std::tuple` and the placeholders could be a `std::integral_constant`. This should make it easier to use another backend without breaking API compatibility.

Some other notes:

- Couldn't the operator macros be provided as base classes? So instead of writing:

template <boost::yap::expr_kind Kind, typename Tuple>
struct user_expr
{
    static const boost::yap::expr_kind kind = Kind;

    Tuple elements;

    // Member operator overloads for operator!().
    BOOST_YAP_USER_UNARY_OPERATOR_MEMBER(logical_not, ::user_expr)
};

The user could write:

template <boost::yap::expr_kind Kind, typename Tuple>
struct user_expr : logical_not<::user_expr>
{
    static const boost::yap::expr_kind kind = Kind;

    Tuple elements;
};

You could also get rid of the need for `::user_expr` by just taking it as a type and using something similiar to `mp_transform` to change the parameters. Or even change `Kind` to be a type of `std::integral_constant<boost::yap::expr_kind, Kind>` so `mp_transform` could be used directly:

template <typename Kind, typename Tuple>
struct user_expr : logical_not<user_expr>
{
    static const boost::yap::expr_kind kind = Kind{};

    Tuple elements;
};

- The customization points seem unnecessary. Why wouldn't the user just use `transform`? Using `transform` also keeps the customizations local. Also, implicit transformations seems like it would lead to suprises(and not good suprises). Are those on the cutting block?

- It would be interesting to provide common transformations, like common subexpression elimination, or dead code elimination, but I guess that is beyond the scope of this library.

- The `op_string` function is missing the case for `expr_ref` and `terminal`. Also, the tests only check for plus.

- The `make_expr_from_tuple` does a move here:

auto make_expr_from_tuple (ExprTemplate<Kind, OldTuple> const & expr, NewTuple && tuple)
{ return ExprTemplate<Kind, NewTuple>{std::move(tuple)}; }

But maybe this should be using `std::forward`?

- Did you attempt to use the library? If so:

Yes, I ran it on clang 3.8 and gcc 7.

- How much effort did you put into your evaluation of the review?

Several hours