- 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
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
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` so `mp_transform` could be used directly:
template
struct user_expr : logical_not
{
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 const & expr, NewTuple && tuple)
{ return ExprTemplate{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
