Re: [PFR] Review part 2

From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2020-10-16 10:15:12

чт, 8 окт. 2020 г. в 02:07, Steven Watanabe via Boost <boost_at_[hidden]>:
>
> 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.

Fixed

> - Can the operators be constexpr?

Needs investigation. Made an issue on github to remember about it

> - 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.

Removed the header

> - `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;
>     }
> }

Changed those to functions, now looks much better

> - 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.

Probably fixed that, but not in a very explicit form.

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

operator<=> in С++20 could be defaulted, so there's no much sense in
adding that functionality to the library

> - 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?

Added helpers and examples on how to do that.

> - How would I define operators for a class template?
>
> detail/fields_count.hpp:

Partially solved via *_fields functions. Created a github issue to
investigate further

> - 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.

They need to do that, because if they are instantiated with an
anonymous type then compilers complain on linkage "used but not
defined in this translation unit, and cannot be defined in any other
translation unit because its type does not have linkage"

> - 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.

Yep, me too

> - 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.

This requires a lot of thinking. Created an issue, will do that later

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

Fixed

> In Christ,
> Steven Watanabe

Many thanks for the review!

-- 
Best regards,
Antony Polukhin