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

Subject: Re: [boost] [yap] Review part 2: implementation
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-15 05:29:12

On Wed, Feb 14, 2018 at 4:45 PM, Steven Watanabe via Boost <
boost_at_[hidden]> 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_at_[hidden]> 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/default_eval.hpp#L136

#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