- 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
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. 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
AMDG On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
<snip>
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).
You don't need a separate macro: #define BOOST_YAP_USER_UNARY_OPERATOR_MEMBER(op_name, ...) \ /* implementation */
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.
In Christ, Steven Watanabe
On Tue, Feb 13, 2018 at 11:28 AM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
AMDG
On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
<snip>
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).
You don't need a separate macro: #define BOOST_YAP_USER_UNARY_OPERATOR_MEMBER(op_name, ...) \ /* implementation */
Ah, that's right. Zach
AMDG On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost@lists.boost.org> wrote:
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.
If that's really important then why doesn't BOOST_YAP_USER_FREE_BINARY_OPERATOR take two expr_template parameters? In Christ, Steven Watanabe
On Tue, Feb 13, 2018 at 11:30 AM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
AMDG
On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost@lists.boost.org> wrote:
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.
If that's really important then why doesn't BOOST_YAP_USER_FREE_BINARY_OPERATOR take two expr_template parameters?
Because there's only one result. The expr_template parameter governs how the result is constructed, not which inputs are accepted. Zach
AMDG On 02/13/2018 09:47 AM, Zach Laine via Boost wrote:
On Tue, Feb 13, 2018 at 11:30 AM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost@lists.boost.org> wrote:
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.
If that's really important then why doesn't BOOST_YAP_USER_FREE_BINARY_OPERATOR take two expr_template parameters?
Because there's only one result. The expr_template parameter governs how the result is constructed, not which inputs are accepted.
Doesn't this make BOOST_YAP_USER_FREE_BINARY_OPERATOR unusable, since expression already uses it, so using it again with a different ExpressionTemplate would be ambiguous? In Christ, Steven Watanabe
On Tue, Feb 13, 2018 at 1:51 PM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
AMDG
On 02/13/2018 09:47 AM, Zach Laine via Boost wrote:
On Tue, Feb 13, 2018 at 11:30 AM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
On 02/12/2018 06:57 PM, Zach Laine via Boost wrote:
On Mon, Feb 12, 2018 at 4:15 PM, P F via Boost <boost@lists.boost.org> wrote:
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.
If that's really important then why doesn't BOOST_YAP_USER_FREE_BINARY_OPERATOR take two expr_template parameters?
Because there's only one result. The expr_template parameter governs how the result is constructed, not which inputs are accepted.
Doesn't this make BOOST_YAP_USER_FREE_BINARY_OPERATOR unusable, since expression already uses it, so using it again with a different ExpressionTemplate would be ambiguous?
Sure, if you use both of them. I expect people only to use expression for quick-and-dirty small-scale uses of Yap, or for experimentation though. However, the same problem exists if I have ET1 and ET2 that want to use BOOST_YAP_USER_FREE_BINARY_OPERATOR. In that case, the same ambiguity exists. The solution is the same as any operator overload ambiguity, I think: sfinae. If there's not already an appropriate constraining macro to replace BOOST_YAP_USER_FREE_BINARY_OPERATOR (I'd have to look around a bit), I should add one. *TODO* Zach
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
participants (3)
-
P F -
Steven Watanabe -
Zach Laine