Boost logo

Boost :

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


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

Boost list run by bdawes at, gregod at, cpdaniel at, john at