On Wed, Feb 14, 2018 at 4:45 PM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
AMDG
On 02/14/2018 03:11 PM, Zach Laine via Boost wrote:
On Tue, Feb 13, 2018 at 4:51 PM, Steven Watanabe via Boost < boost@lists.boost.org> wrote:
I've snipped most of this email. The missing parts were either addressed last night in my local copy of the code, or are now GitHub tickets.
algorithm.hpp:
221: template <long long I, typename Expr> decltype(auto) get (Expr && expr, hana::llong<I> i); Does this need to be restricted to hana::llong, rather than, say, std::integral_constant?
It's the lack of nice syntax for integral_constant literals that made me choose this. I write get(expr, N_c) a lot, and I expect users to as
well.
Supporting an IntegralConstant doesn't mean that you can't pass 0_c.
Is the change from std::integral_constant to IntegralConstant significant? That is, are you asking that I accept models of some concept, or just std::integral_constant?
300: get_c: hana uses at/at_c. If you're going to use get, I'd prefer to use get<0> rather than get_c<0> to match the naming in the standard library.
I prefer what Hana does to the get() from the standard.
Then call it at rather than get. If you're using a well-known name, then you should follow the existing conventions for that function.
I'm with you in the general case. I think get() is overloaded to the point of having lost any specific meaning though. There's the std::get() overloads that operate on tuple-like types, but there are also numerous other get()s in the wild, including the somewhat similar ones from Boost.Fusion. I don't know of any that take an integral constant as a second parameter. I don't think this is a significant source of confusion.
<snip>
expression_free_operators.hpp/expression_if_else.hpp:
- Why is this not part of expression.hpp?
For the same reason that expression_free_operators.hpp is not. Those three headers are separate pieces that you may want some or all of, so I kept them separate.
expression_if_else makes a certain amount of sense as being separate. Making expression_free_operators separate comes with a high risk of ODR problems in any code that has different behavior depending on whether a given operator is defined. It's extra strange because expression_free_operators.hpp only has half the operator overloads, with the other half being defined as members.
I've read this a few times and don't get it. Without including expression_free_operators.hpp, where might the other ODR-colliding operators come from? Do you mean if the user defines her own operators for expression<>, and then includes expression_free_operators.hpp in another TU, or something else? [snip] Let me bring operator||/operator&& short circuiting back up. After implementing it, I don't think it's appropriate. Consider the current non-short-circuiting implementation on the boost_review branch: https://github.com/tzlaine/yap/blob/boost_review/include/boost/yap/detail/de... #define BOOST_YAP_BINARY_OPERATOR_CASE(op, op_name) \ template <> \ struct default_eval_expr_impl<expr_kind:: op_name> \ { \ template <typename Expr, typename ...T> \ decltype(auto) operator() (Expr && expr, T && ... args) \ { \ using namespace hana::literals; \ return \ default_eval_expr(static_cast<Expr &&>(expr).elements[0_c], static_cast<T &&>(args)...) op \ default_eval_expr(static_cast<Expr &&>(expr).elements[1_c], static_cast<T &&>(args)...); \ } \ }; If the "&&" in "default_eval_expr(...) && default_eval_expr(...)" resolves to a built-in operator&&(), short-circuiting will happen as always, right? But if the "&&" resolves to a user-defined operator&&(), let's say this one: struct my_type { // ... std::shared_ptr<int> operator&&(my_type const & rhs) {/*...*/} }; then I really don't want to artificially impose the short-circuiting. The whole point of evaluate() is that it evaluates to whatever your Yap expression would have if it were a plain ol' non-Yap expression.
Thanks again for the detailed review!
Will there by chance be a part 3?
Yes. I still have the tests + a summary. You may have noticed that I haven't given a vote yet. I was planning to finish it quickly today, but since Louis extended the review, I can be a bit more thorough.
Sweet. Looking forward to it. Zach