AMDG
- Please include the standard copyright/licence text in
all files.
- headers should be in include/boost
algorithm.hpp:
80: in deref:
"std::is_rvalue_reference<Expr>{} &&"
This is always false, since Expr will be deduced as
either E or E&. That's probably for the best, though,
as I don't think moving is ever right here.
110: ADL
180: in value_impl
(ValueOfTerminalsOnly && kind == expr_kind::terminal)
Testing ValueOfTerminalsOnly is redundant.
214: in value
- Otherwise, \a x is forwarded to the caller. */
Does value actually make sense sense for non-unary
expressions?
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?
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.
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.
414: in callable:
detail::arity_of<kind>() == detail::expr_arity::n,
Shouldn't this check whether kind is call_expr?
439: in argument:
"In argument(expr, I), I must be nonnegative, and less "
"than hana::size(expr.elements)."
The assertion text doesn't match the code. I think it
would be better to make the message more abstract, rather
than trying to describe the exact conditions numerically.
667-669:
/** Returns the result of transforming (all or part of) \a
expr using whatever overloads of
<code>Transform::operator()</code> that match \a
expr.
Remove "that" before "match." As written "whatever" is
left hanging.
698: inline char const * op_string (expr_kind kind)
Does this belong here and not in print.hpp?
algorithm_fwd.hpp:
17: expr_ref, ///< An (possibly \c const) reference
to another expression.
s/An/A/
config.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.
expression.hpp:
11-13: \note Due to a limitation of Doxygen, each of the
<code>value()</code>, <code>left()</code>, <code>right()</code>, and
operator overloads listed here is a stand-in for three member
Did you try \overload? That sometimes helps.
49: eval_expression_as is particularly insidious here as
it uses static_cast, thus enabling even conversions
that would normally be explicit.
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.
expression_free_operators.hpp/expression_if_else.hpp:
- Why is this not part of expression.hpp?
operators.hpp: No comments.
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.
121,176: ADL
144,146,185,187: ADL (Is this intentional? It isn't a documented
customization point)
user_macros.hpp:
8: # define BOOST_PP_CAT(lhs, rhs) BOOST_PP_CAT_O((lhs, rhs))
This is obviously copy-pasted from Boost.Preprocessor.
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?
315: in BOOST_YAP_USER_FREE_BINARY_OPERATOR
auto operator BOOST_YAP_INDIRECT_CALL(op_name)() (T && lhs, Expr &
rhs) \
Do you really want this to match even non-expr lvalues in the rhs?
Oh, I see, it's handled by SFINAE in free_binary_op_result_t.
Still, you should probably handle the lvalue and rvalue cases
in the same way.
467: in BOOST_YAP_UDT_UNARY_OPERATOR
auto operator BOOST_YAP_INDIRECT_CALL(op_name)((T && x))
This doesn't work for post-inc/decrement.
619: ::boost::hana::ic_detail::parse({c...})
Calling another library's details is a big red flag.
detail/algorithm.hpp:
20: struct static_const
This dance is no longer necessary in C++17.
variables can be declared `inline`.
44: struct partial_decay;
The documentation seems to imply that you also decay
function references.
192-200: This specialization is unnecessary as the
primary template (undefined) works well enough.
detail/default_eval.hpp:
149,302: BOOST_YAP_BINARY_OPERATOR_CASE(logical_or) // ||
150,303: BOOST_YAP_BINARY_OPERATOR_CASE(logical_and) // &&
No short circuit evaluation.
162,310: return eval_comma(
Please tell me that I'm reading this wrong and that
(a) this can safely return void, and (b) the left
argument is evaluated before the right argument.
192,332: return eval_if_else(
It looks like this always evaluates both branches.
Additional notes that I missed previously:
concepts.html:
- There's an additional constraint on Expression that
you do not state explicitly. E e{t} where t is a Tuple,
must store t in e.elements.
In Christ,
Steven Watanabe
