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

Subject: Re: [boost] [yap] Review part 2: implementation
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2018-02-13 22:51:49

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