Re: [boost] [yap] Review part 2: implementation

15 Feb 2018

      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 
     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              \
        {                                                               \
            template                      \
            decltype(auto) operator() (Expr && expr, T && ... args)     \
            {                                                           \
                using namespace hana::literals;                         \
                return                                                  \
                    default_eval_expr(static_cast(expr).elements[0_c], static_cast(args)...) op \
                    default_eval_expr(static_cast(expr).elements[1_c], static_cast(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