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@lists.boost.org> 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
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