Re: [boost] [yap] Review

Subject: Re: [boost] [yap] Review
From: P F (pfultz2_at_[hidden])
Date: 2018-02-14 02:24:22

> On Feb 12, 2018, at 7:57 PM, Zach Laine via Boost <boost_at_[hidden]> wrote:
> 
> 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.

Actually, the main algorithms you use are transform, for_each, and unpack, which is trivial to do with Fit. Doing a `zip` or `filter` is a little more complicated, but grepping the source code, I did not see that. I discuss how thats done here:

http://pfultz2.com/blog/2015/09/12/power-of-unpack/ <http://pfultz2.com/blog/2015/09/12/power-of-unpack/>

Which we can discuss more offline.

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

But Hana should be able to work std::tuple, just the same. So at least changing to use something else, whether Fit or not, you won’t break API compatibility.

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

I am hoping to finish the changes this week to get it into next release.

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

I guess its just a matter of deciding which you hate more inheritance or macros.

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

The point is to use `mp_assign` to change the parameters. So instead of writing:

expr_template<::boost::yap::expr_kind::op_name, tuple_type>

You would write:

mp_assign<expr_type, mp_list<::boost::yap::expr_kind::op_name, tuple_type>

Of course, to make that work, you will need to make `boost::yap::expr_kind::op_name` a type(which can be done by simply wrapping it in an integral constant).

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

Being able to use `mp_assign` to change the parameters to `user_expr`. Although, you could write your own version which allows the first parameter to be an integral.

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

That should still work. I dont see why it wouldn’t. In fact, you should be able to do all of these:

template<boost::yap::expr_kind Kind>
using expr_kind_c = std::integral_constant<boost::yap::expr_kind, Kind>;

constexpr if (kind == expr_kind_c<foo>{})
constexpr if (Kind{} == expr_kind_c<foo>{})
constexpr if (kind{} == expr_kind::foo)

What may not work is doing something like `fit::if_(Kind{} == expr_kind_c<foo>{})`(due to lack of operator overloading for integral_constant), but using `fit::if_c<Kind{} == expr_kind_c<foo>{}>` still does work.

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

I see, I just the forwarding reference, but I see that it will always be an rvalue anyways.

> 
> Zach
> 
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost