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 <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? 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<expr_kind::terminal, hana::tuple<T>> 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<decltype(*this)>; \ using lhs_type = ::boost::yap::detail::operand_type_t<expr_template, this_type const &>; \ 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<sizeof...(c)>({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<R(A...)>; 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