On Feb 12, 2018, at 7:57 PM, Zach Laine via Boost <boost@lists.boost.org> wrote:
On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost@lists.boost.org> 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