Re: [PFR] Review part 2

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2020-10-07 23:06:29

AMDG

I vote to ACCEPT PFR into Boost.

global_ops.hpp/ops.hpp/functions_for.hpp:

- Defining functions in a header as static risks UB via the
  one definition rule.
- Can the operators be constexpr?
- I don't think that global_ops.hpp is a good idea.
  `using namespace boost::pfr::ops` in the global namespace achieves
  almost the same effect and is more clear.
- `using namespace boost::pfr::ops` is also quite fragile.  It
  will effectively declare the operators in the global namespace
  The nearest namespace that encloses both the current scope and
  the boost::pfr::ops).  This means that they will be hidden by
  any operators defined in the current namespace.  For this reason,
  `using boost::pfr::ops::specific operator` is generally more
  reliable for functions like comparison operators that tend to be
  heavily overloaded.

#include <boost/pfr/precise/ops.hpp>

namespace ns {
    struct X {};
    // Uncommenting this causes a compile error
    // bool operator<(X, X) { return false; }
    struct Y {};

    void test() {
        Y y;
        using namespace boost::pfr::ops;
        y < y;
    }
}

- Does the documentation specify anywhere what operators the
  struct members need to define in order for the precise operators
  to work?  It looks like the comparison operators require operator==
  and the operator being defined.

- Would it make sense to define operator<=> for C++20?

- It would be nice if there were a way to select operators
  more specifically for BOOST_PFR_FUNCTIONS_FOR.  What if I
  only want the comparision operators and hash, and want to
  define my own stream operators?

- How would I define operators for a class template?

detail/fields_count.hpp:

- Do ubiq_lref_constructor and ubiq_rref_constructor need to have
  their conversion operators defined?  If they were only declared
  they wouldn't need to use unsafe_declval.

-----------------

- The error when a struct has an array member is not very informative.
  I can't think of a good way to detect this, however.
- It would also be nice if there were a trait that covers
  all the detectable modes of failure, or perhaps a macro that
  has all the static asserts in fields_count.

- The reference docs often uses it's instead of its:
  its = possessive pronoun
  it's = contraction of it is

In Christ,
Steven Watanabe