Re: [boost] [yap] Review

Subject: Re: [boost] [yap] Review
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-13 01:57:34

On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost_at_[hidden]>
wrote:

Thanks for reviewing, Paul.

[snip]

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

 I actually experimented with trying to remove the Hana bits for this exact
reason.  The conclusion I came to was that writing the library was quite a
bit harder in a couple of places, but that's not too bad.  The bigger
problem is that the lack of algorithms over std::tuple for users of the
library when they write transforms.  Maybe we should talk offline about how
to do things that are done now with Hana, with Fit instead.
C++14-including-MSVC or even C++11 would be great.  It will have to be a
pretty clear win for users for me to make that change, though, given how
great Hana is for working with tuples.

In either case, I really want Fit for some things in Yap.  When can we
expect it in a release?  Nudge! :)

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

Visibly using inheritance for code reuse goes against all I hold dear.  One
pragmatic problem with this is that types in the debugger that have been
assembled from a dozen or so operator-providing base classes shows up as
multiple pages of garbage (at least in my preferred debugger, gdb).

You could also get rid of the need for `::user_expr`

We actually want that.  It's often necessary to be able to return an
expression created from some template different than the template defining
he operator.  I could have doubled the number of functions and macros in
Yap that take an expression template template parameter, providing an
overload that does not take this parameter.  Of course, the macros would be
differently named, not overloaded (another small problem).  This would make
it easier to write the common-case code (in which the expression template
used for the return type is that same as the expression template defining
the operation).  I deemed that to be a bad trade-off.

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

You certainly could.  What does this buy, though?  I'm not getting that
part yet.  One thing this give up is the inability to write functions in
terms of chained

constexpr if (kind == expr_kind::foo)

statements, which I quite like.  You can do this with is_same<> of course,
but at the cost of 45 extra template instantiations and a lot more noise.

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

Absolutely.  As I said in Steven's review, they've only made it this long
to make sure people were not more interested in them than I am (which is
not at all).

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

I think it is.

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

That function only applies to operators, not those special expr_kinds, so
those were left out.  I added them for completeness, and adjusted the docs
for that function accordingly.

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

No, its in detail::, and its only used in this one line:

return make_expr_from_tuple(expr, std::move(transformed_tuple));

So a move is always what we want.

Zach