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

Subject: Re: [boost] [yap] Review part 2: implementation
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2018-02-14 22:11:40

On Tue, Feb 13, 2018 at 4:51 PM, Steven Watanabe via Boost <
boost_at_[hidden]> wrote:

> AMDG
>
>
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:
>
> 214: in value
>         - Otherwise, \a x is forwarded to the caller. */
>    Does value actually make sense sense for non-unary
>    expressions?
>

I hope so.  I added this because I found that the semantics of value()
including the non-unary case are looser, it is still useful in this form in
generic code.  It saves you from having to handle unary-vs.-non-unary as
separate cases in a lot of code that is essentially passing through a
parameter and doesn't want to have to care about what it is.  value() did
not originally work like this, but I found it harder to use then, so I
added the non-unary pass-through.

> 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.

> 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.  Also, the get()
from the standard is heavily overloaded, and I'd like not to match its
signature, if only for clarity.  For instance, after a quick glance at the
code, what does get<3>(x) do?  I dislike that, in part due to ADL,  I
really have no idea without looking somewhere else.

> 317: in left:
>             kind == expr_kind::expr_ref ||
>             detail::arity_of<kind>() == detail::expr_arity::two,
>    This assertion isn't quite strict enough as it
>    will accept an expr_ref to anything.  I think
>    you need a get_kind_after_unwrapping_expr_refs
>    function, as this seems to be needed in quite
>    a few places.
>

That would provide better locality of error messages.  However, you'll
still get an error, just a call or two down the call stack.  It's a lot of
compile-time mechanism to get an improvement, and this slows down all the
non-error cases.  I don't think it's a favorable trade.

> algorithm_fwd.hpp:
>
> #ifndef BOOST_NO_CONSTEXPR_IF
> /** Indicates whether the compiler supports constexpr if.
>
>     If the user does not define any value for this, we assume that the
>     compiler does not have the necessary support.  Note that this is a
>     temporary hack; this should eventually be a Boost-wide macro. */
> #define BOOST_NO_CONSTEXPR_IF 1
> #endif
>    This means that BOOST_NO_CONSTEXPR_IF will always be
>    defined (presumably to 0 or 1).  Unfortunately, you
>    will never use if constexpr because you always test
>    the macro with #ifdef instead of #if.
>

So frustrating!  I noticed this a long time ago, and then got distracted
and forgot to go back and fix it.  Thanks.

> expression.hpp:
>
> 49: eval_expression_as is particularly insidious here as
>     it uses static_cast, thus enabling even conversions
>     that would normally be explicit.
>

Agreed.  This is going to be cut though, based on earlier comments.

> 266:     template <typename T>
>          struct expression<expr_kind::terminal, hana::tuple<T>>
>    The only differences that I can see between this specialization
>    and the primary template are
>    - terminal has an extra constructor expression(T &&).
>    - terminal does not have left/right.
>    I don't think either of these is worth making a specialization.
>    make_terminal is more convenient and left and right will
>    already fail in the static assert.
>

That ctor is pretty important.  You could use make_terminal, as you said,
but the main use case for expression<> is prototyping, in which case I'd
like to have a template that as closely as possible matches the expression
template you'll eventually write in your post-prototype implementation.
Explicit terminal types or templates are almost always the right answer in
such cases, and I think expression<> should reflect that.

> 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.

> print.hpp:
>
> 60: inline bool is_const_expr_ref (...) { return false; }
>    If the argument has a non-trivial copy/move/destructor
>    then this is conditionally supported with implementation
>    defined semantics. [expr.call]  Don't use the elipsis
>    like this if you want to write portable code.
>

I did no know that; thanks.  Fixed.

> 121,176: ADL
>
> 144,146,185,187: ADL (Is this intentional?  It isn't a documented
>    customization point)
>

No, this and the other ADL holes were all unintentional.  Fixed.

> 89-90:
>         using this_type =
> ::boost::yap::detail::remove_cv_ref_t<decltype(*this)>; \
>         using lhs_type =
> ::boost::yap::detail::operand_type_t<expr_template, this_type const &>; \
>    Is there a reason why you're stripping off qualifiers and
>    then adding them back again?
>

It was simply for consistency among the three different overloads each
macro provides.  I did it I think when there was a lot more going on in
each macro, to keep from getting confused.  I've removed them now though.

> 619: ::boost::hana::ic_detail::parse<sizeof...(c)>({c...})
>   Calling another library's details is a big red flag.
>

Well, it's my maintenance burden, no?  I think it beats copy/pasting, since
the implementation is quite stable, and since I already include the
relevant header anyway.

> detail/algorithm.hpp:
>
> 20:  struct static_const
>   This dance is no longer necessary in C++17.
>   variables can be declared `inline`.
>

Yeah, but this library supports C++14 too.  Anyway, the customization
points are going away.

Thanks again for the detailed review!

Will there by chance be a part 3?

Zach