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

Subject: Re: [boost] [yap] Review part 2: implementation
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-02-14 22:45:55

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.

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

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

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

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

  It used to be flat-out undefined behavior, but
C++11 relaxed the rules somewhat.  Note that it's
safe in an unevaluated context, which (in my
experience) is a more common use of the ellipsis
in C++ code.

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

In Christ,
Steven Watanabe