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

On Tue, Feb 13, 2018 at 4:51 PM, Steven Watanabe via Boost < boost@lists.boost.org> 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 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 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; \ using lhs_type = ::boost::yap::detail::operand_type_t; \ 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({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