[FPR] Review of FPR starts Today: Sept 28 - Oct 7 - Boost

[FPR] Review of FPR starts Today: Sept 28 - Oct 7

Benedek Thaler

27 Sep 2020 27 Sep '20

6:16 p.m.

The Boost formal review of the FPR (Flat Precise Reflection, ex Magic Get, ex PODs Flat Reflection) starts Today, taking place from September 28, 2020 to October 7, 2020. The library is authored by Antony Polukhin, author of Boost.DLL, Stacktrace, Type Index libraries. Documentation: http://apolukhin.github.io/magic_get/ Source: https://github.com/apolukhin/magic_get The library is meant for accessing structure elements by index and providing other std::tuple like methods for user defined types without any macro or boilerplate code. The FPR documentation summarizes the out-of-the-box added functionality for aggregate initializable structures: - comparison operators - heterogeneous comparators - hash - stream operators - access to members by index - member reflections - methods for cooperation with std::tuple - methods to visit each field of the structure If the description isn't immediately obvious for you, here's a motivating example: // requires: C++14 #include <iostream> #include <string> #include "boost/pfr/precise.hpp" struct some_person { std::string name; unsigned birth_year; }; int main() { some_person val{"Edgar Allan Poe", 1809}; std::cout << boost::pfr::get<0>(val) // No macro! << " was born in " << boost::pfr::get<1>(val); // Works with any aggregate initializables! } Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including FPR as a Boost library. Please be explicit about your decision (ACCEPT or REJECT). Some other questions you might want to consider answering: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html Thank you for your effort in the Boost community. Benedek - review manager of the FPR library

Show replies by date

Vinnie Falco

28 Sep 28 Sep

4:29 p.m.

On Sun, Sep 27, 2020 at 11:17 AM Benedek Thaler via Boost wrote:

...

...

Should the announcement also go to boost-users? Thanks

Andrey Semashev

5:12 p.m.

On 9/27/20 9:16 PM, Benedek Thaler via Boost wrote:

...

The Boost formal review of the FPR (Flat Precise Reflection, ex Magic Get, ex PODs Flat Reflection) starts Today, taking place from September 28, 2020 to October 7, 2020.

I assume it's PFR, not FPR? At least, that's what GitHub project page and docs say.

Benedek Thaler

5:23 p.m.

On Mon, Sep 28, 2020, 19:12 Andrey Semashev via Boost wrote:

...

On 9/27/20 9:16 PM, Benedek Thaler via Boost wrote:

...
The Boost formal review of the FPR (Flat Precise Reflection, ex Magic Get, ex PODs Flat Reflection) starts Today, taking place from September 28, 2020 to October 7, 2020.

I assume it's PFR, not FPR? At least, that's what GitHub project page and docs say.

It really is. My mistake.

...

Richard Hodges

2 Oct 2 Oct

6:02 p.m.

Dear all, My preliminary PFR review: For this review I have created a github repository for testing. I will continue to update this and flag issues against the magic_get repo as I find them. * - What is your evaluation of the design?* The design principles are sane and in my view most use cases are covered. However, I feel the library is currently at MVP or PoC stage and is not yet fully complete. The one feature I miss is the ability to zip a sequence of strings to the enumeration of elements, since given this statement: std::cout << fido << '\n'; I may wish to see this output: { name="fido", species="lupus lupus", temperament=aloof } rather than this: {"fido", "lupus lupus", aloof} The third element being unquoted was interesting to me as in my test, fido is an object of type animal, who's definition looks like this: struct animal { std::string name; std::string_view species; boost::string_view temperament; }; *test-1:* see: https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-1.cpp Looking into the code I found that there are overloads of quoted_helper(T&&) in the pfr::detail namespace in order to ensure proper quoting of std::string and (if present) std::string_view. However, there is no means for me to provide a customisation of the quoting concept for unsupported types that I might want to quote (such as boost::string_view, std::exception, json::string, etc). In my view this is a design oversight. * - What is your evaluation of the implementation?* I was able to produce an interesting compile error with this structure when using BOOST_PFR_xxx_FUNCTIONS_FOR. *test-2 and test-3:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-2.cpp Seems to produce infinite recursive template expansion https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-3.cpp Complains about lack of std::hash for boost::string_view I suspect these two errors are linked. My suggestion would be that BOOST_PFR_xxx_FUNCTIONS_FOR should either: a) be split down into EQUALITY, STREAMABLE, ORDERED, HASH_EQUALITY etc, or b) only seek to create the operators that are legal for all discovered types. My reasoning here is that since I like to log objects at trace level, I will probably always want to use BOOST_PFR_PRECISE_FUNCTIONS_FOR on every type I create. Not all types are hashable or comparable, but it is reasonable to demand that most types are streamable. *test-4:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-4.cpp This may be controversial, but I wonder whether it would be a good idea to provide specialisations for operations on containers in the pfr::ops namespace, or a sub-namespace of it. Given: struct family { std::vector<animal> members; } f; the following code will not compile because of the lack of streaming operator for std::basic_vector<> int main() { using namespace boost::pfr::ops; std::cout << f << '\n'; } For my anticipated use case of this library, this would be a common requirement, and having to write an overload of operator<< for family would defeat the purpose of using PFR. *test-5:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-5.cpp flat_structure_tie doesn't work with const references. this line refused to compile: auto& [a, b, c] = boost::pfr::flat_structure_tie(n); when n was const. It was fine when n was a mutable lvalue. * - What is your evaluation of the documentation?* A little sparse. I had to discover the above cases for myself. The documentation certainly needs a "reference" section with links to a full description of each function or type. * - What is your evaluation of the potential usefulness of the library?* Extremely useful. I can't tell you how bored I am of writing overloads for comparison and streaming! * - Did you try to use the library? With which compiler(s)? Did you have any problems?* I compiled on gcc 9.3 in c++17 mode * - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?* I read the documentation, read briefly through the source code and created a repo to try things out that I felt would be useful to me. The repo is here: https://github.com/madmongo1/pfr_review * - Are you knowledgeable about the problem domain?* I think we all are. The lack of reflection in C++ is absolute proof that design-by-committee is doomed to failure. *Regarding Acceptance:* My view is that PFR will be an extremely welcome and useful addition to my application development toolkit, and I would very much like to see it in boost. However, as mentioned, it seems to me that the current state of the library is that it is a good foundation for further development. Including it right now will I believe result in many users being caught in unexpected gotchas (like not being able to tie a const reference). This prevents me from pushing the ACCEPT button today. I am currently in the camp of CONDITIONALLY ACCEPT, with the condition that the shortcomings I was able to find are addressed and more common use cases are given coverage in the test suite. On Sun, 27 Sep 2020 at 20:16, Benedek Thaler via Boost < boost@lists.boost.org> wrote:

...

The Boost formal review of the FPR (Flat Precise Reflection, ex Magic Get, ex PODs Flat Reflection) starts Today, taking place from September 28, 2020 to October 7, 2020.

The library is authored by Antony Polukhin, author of Boost.DLL, Stacktrace, Type Index libraries.

Documentation: http://apolukhin.github.io/magic_get/ Source: https://github.com/apolukhin/magic_get

The library is meant for accessing structure elements by index and providing other std::tuple like methods for user defined types without any macro or boilerplate code.

The FPR documentation summarizes the out-of-the-box added functionality for aggregate initializable structures:

- comparison operators - heterogeneous comparators - hash - stream operators - access to members by index - member reflections - methods for cooperation with std::tuple - methods to visit each field of the structure

If the description isn't immediately obvious for you, here's a motivating example:

// requires: C++14 #include <iostream> #include <string> #include "boost/pfr/precise.hpp"

struct some_person { std::string name; unsigned birth_year; };

int main() { some_person val{"Edgar Allan Poe", 1809};

std::cout << boost::pfr::get<0>(val) // No macro! << " was born in " << boost::pfr::get<1>(val); // Works with any aggregate initializables! }

Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including FPR as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).

Some other questions you might want to consider answering:

- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?

More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html

Thank you for your effort in the Boost community.

Benedek - review manager of the FPR library

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

-- Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212

Hadriel Kaplan

6:21 p.m.

...

On Oct 2, 2020, at 2:02 PM, Richard Hodges via Boost wrote:

Looking into the code I found that there are overloads of quoted_helper(T&&) in the pfr::detail namespace in order to ensure proper quoting of std::string and (if present) std::string_view.

However, there is no means for me to provide a customisation of the quoting concept for unsupported types that I might want to quote (such as boost::string_view, std::exception, json::string, etc). In my view this is a design oversight.

Heh, that was the first thing I asked for on the GitHub issues list. :) See: https://github.com/apolukhin/magic_get/issues/47 https://github.com/apolukhin/magic_get/issues/47 There’s an example in that issue for how to create your own version - not for specializing each of your types exactly, but rather re-implementing a bit of PFR's streaming code in your own namespace. Once you do that, you can do whatever you want - including handling your own custom types and your own customization-point model (i.e., by implementing template specializations, or custom function names, or tag-dispatching, or whatever). I used it for making `bool` types print “true” or “false” instead of “1” or “0", for example - instead of having to pass `std::boolalpha` into `std::ostream` _everywhere_. -hadriel

Antony Polukhin

6:47 p.m.

On Fri, Oct 2, 2020, 21:03 Richard Hodges via Boost wrote:

...

Dear all,

My preliminary PFR review:

For this review I have created a github repository for testing. I will continue to update this and flag issues against the magic_get repo as I find them.

* - What is your evaluation of the design?*

The design principles are sane and in my view most use cases are covered. However, I feel the library is currently at MVP or PoC stage and is not yet fully complete.

The one feature I miss is the ability to zip a sequence of strings to the enumeration of elements, since given this statement:

std::cout << fido << '\n';

I may wish to see this output:

{ name="fido", species="lupus lupus", temperament=aloof }

rather than this:

{"fido", "lupus lupus", aloof}

That feature was requested, but nobody knows how to implement it without macro and with computational complexity less than O(N!). Any hints are welcome! Otherwise the Describe library from Peter Dimov would be helpful (it is not reviewed yet) The third element being unquoted was interesting to me as in my test, fido

...

is an object of type animal, who's definition looks like this:

struct animal { std::string name; std::string_view species; boost::string_view temperament; };

*test-1:* see: https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-1.cpp

Looking into the code I found that there are overloads of quoted_helper(T&&) in the pfr::detail namespace in order to ensure proper quoting of std::string and (if present) std::string_view.

However, there is no means for me to provide a customisation of the quoting concept for unsupported types that I might want to quote (such as boost::string_view, std::exception, json::string, etc). In my view this is a design oversight.

There's actually a way to customize printing by writing a generic printing function you need. See operator<< definition here https://github.com/apolukhin/magic_get/issues/47#issuecomment-700159578 But it's an oversight that such customization is not shown in docs. I'll fix that soon. * - What is your evaluation of the implementation?*

...

I was able to produce an interesting compile error with this structure when using BOOST_PFR_xxx_FUNCTIONS_FOR.

*test-2 and test-3:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-2.cpp Seems to produce infinite recursive template expansion

https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-3.cpp Complains about lack of std::hash for boost::string_view

Many thanks for the test cases! I'll take a deeper look and add those to the testsuite soon. I suspect these two errors are linked. My suggestion would be

...

that BOOST_PFR_xxx_FUNCTIONS_FOR should either: a) be split down into EQUALITY, STREAMABLE, ORDERED, HASH_EQUALITY etc, or

There's a way to do that, and you do not need a macro. Just write an operator and use one of the functors from boost/pfr/precise/functors.hpp . This requires almost the same amont of typing, but avoids ugly macro. b) only seek to create the operators that are legal for all discovered

...

types.

Good point. A bunch of detectors already available here https://github.com/apolukhin/magic_get/blob/develop/include/boost/pfr/detail... , but those are used only for boost::pfr::ops namespace. I'll try to use those to the *FUNCTIONS_FOR. My reasoning here is that since I like to log objects at trace level, I

...

will probably always want to use BOOST_PFR_PRECISE_FUNCTIONS_FOR on every type I create. Not all types are hashable or comparable, but it is reasonable to demand that most types are streamable.

*test-4:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-4.cpp

This may be controversial, but I wonder whether it would be a good idea to provide specialisations for operations on containers in the pfr::ops namespace, or a sub-namespace of it.

Given:

struct family { std::vector<animal> members; } f;

the following code will not compile because of the lack of streaming operator for std::basic_vector<>

int main() { using namespace boost::pfr::ops; std::cout << f << '\n'; }

For my anticipated use case of this library, this would be a common requirement, and having to write an overload of operator<< for family would defeat the purpose of using PFR.

That's doable, but I'm not sure that this is going to be a good idea. There are some crazy self recursive like types (*::filesystem::path for example), and iterating over their range could get you into infinite recursion. I need to do more experiments. *test-5:*

...

https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-5.cpp

flat_structure_tie doesn't work with const references.

this line refused to compile:

auto& [a, b, c] = boost::pfr::flat_structure_tie(n);

when n was const. It was fine when n was a mutable lvalue.

The main use case for this function is something like this: struct my_struct { int i, short s; }; my_struct s; flat_structure_tie(s) = std::tuple{10, 11}; assert(s.s == 11); But you are right, there may be users who need this function for const values. I'll add an overload. * - What is your evaluation of the documentation?*

...

A little sparse. I had to discover the above cases for myself. The documentation certainly needs a "reference" section with links to a full description of each function or type.

Please elaborate. Is http://apolukhin.github.io/magic_get/boost_pfr/short_examples_for_the_impati... missing something? Or some different approach should be used? * - What is your evaluation of the potential usefulness of the library?*

...

Extremely useful. I can't tell you how bored I am of writing overloads for comparison and streaming!

* - Did you try to use the library? With which compiler(s)? Did you have any problems?*

I compiled on gcc 9.3 in c++17 mode

* - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?*

I read the documentation, read briefly through the source code and created a repo to try things out that I felt would be useful to me. The repo is here: https://github.com/madmongo1/pfr_review

Many thanks! Especially for the Boost licensed test cases! * - Are you knowledgeable about the problem domain?*

...

I think we all are. The lack of reflection in C++ is absolute proof that design-by-committee is doomed to failure.

*Regarding Acceptance:*

My view is that PFR will be an extremely welcome and useful addition to my application development toolkit, and I would very much like to see it in boost. However, as mentioned, it seems to me that the current state of the library is that it is a good foundation for further development. Including it right now will I believe result in many users being caught in unexpected gotchas (like not being able to tie a const reference). This prevents me from pushing the ACCEPT button today.

I am currently in the camp of CONDITIONALLY ACCEPT, with the condition that the shortcomings I was able to find are addressed and more common use cases are given coverage in the test suite.

If you have some more test cases - please share :)

...

Antony Polukhin

6 Oct 6 Oct

11:19 a.m.

пт, 2 окт. 2020 г. в 21:03, Richard Hodges via Boost : <...>

...

*test-1:* see: https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-1.cpp

Looking into the code I found that there are overloads of quoted_helper(T&&) in the pfr::detail namespace in order to ensure proper quoting of std::string and (if present) std::string_view.

However, there is no means for me to provide a customisation of the quoting concept for unsupported types that I might want to quote (such as boost::string_view, std::exception, json::string, etc). In my view this is a design oversight.

Added an example into the docs on customization of printing: http://apolukhin.github.io/magic_get/boost_pfr/tutorial.html#boost_pfr.tutor...

...

*test-2 and test-3:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-2.cpp Seems to produce infinite recursive template expansion

The example should not compile, because the nested types are not aggregates. In C++14 it gives a correct static assert ../../../boost/pfr/detail/core14_classic.hpp:509:5: error: static_assert failed "====================> Boost.PFR: Type can not be used is flat_ functions, because it's not POD" in C++17 it also gives a correct static assert ../../../boost/pfr/detail/fields_count.hpp:225:9: error: static assertion failed: ====================> Boost.PFR: Type must be aggregate initializable. Clang crashes after that. GCC goes into an infinite loop, ignoring all the template instantiation depth limitations and static_assert messages printing. Using BOOST_PFR_PRECISE_FUNCTIONS_FOR now gives a complaint of missing sd::hash specialization: ../../../boost/pfr/detail/functional.hpp:133:9: error: static_assert failed due to requirement 'sizeof(boost::basic_string_view) && false' "====================> Boost.PFR: std::hash not specialized for type T" I've added the tests and will report issues to the compiler developers soon. If the behavior is surprising for you, please elaborate on expected behavior.

...

https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-3.cpp Complains about lack of std::hash for boost::string_view

Made the complaint more explicit.

...

*test-4:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-4.cpp

This may be controversial, but I wonder whether it would be a good idea to provide specialisations for operations on containers in the pfr::ops namespace, or a sub-namespace of it.

Given:

struct family { std::vector<animal> members; } f;

the following code will not compile because of the lack of streaming operator for std::basic_vector<>

int main() { using namespace boost::pfr::ops; std::cout << f << '\n'; }

For my anticipated use case of this library, this would be a common requirement, and having to write an overload of operator<< for family would defeat the purpose of using PFR.

The library does not intend to provide ostream operators for any type. This is out of the library scope.

...

*test-5:* https://github.com/madmongo1/pfr_review/blob/master/pre-cxx20/test-5.cpp

flat_structure_tie doesn't work with const references.

this line refused to compile:

auto& [a, b, c] = boost::pfr::flat_structure_tie(n);

when n was const. It was fine when n was a mutable lvalue.

Fixed -- Best regards, Antony Polukhin

Steven Watanabe

3 Oct 3 Oct

11:21 a.m.

New subject: [PFR] Review part 1 - documentation

AMDG index.html - "(Boost.PFR) adds following out-of-the-box" "...adds /the/ following..." - "member reflections" Make "reflection" singular rather than plural. - "and the library would work fine" "will work fine" would be better. The subjunctive sounds awkward. short_examples_for_the_impatient.html: - s/constains/contains/ (several places) tutorial.html: - "Flat or Precise functions to choose" Maybe "Choosing Flat or Precise functions" or "How to choose Flat or Precise Functions" or "Should I use Flat or Precise functions?" - "There are three ways to start using Boost.PFR hashing, comparison and streaming operators" Is hashing an operator? - "Each method has it's own drawbacks and suits own cases." s/it's/its/ "...suits /its/ own cases" - "...use operators from Boost.PFR only if there's no operators" "there's" does not agree with "operators" (wrong number) - "Argument Dependant Lookup works well, std::less will find the operators..." Comma splice - "BOOST_PFR_FLAT_FUNCTIONS_FOR(T) can not be used for local types, it must be called only once in namespace of T" Comma splice - global_ops.hpp I'm not quite sure how this makes argument dependent lookup work except for types declared in the global namespace. Even if you #include the header before the code that calls the operators, they can still be hidden. limitations.html: - "all it's fields must be constexpr default constructible" s/it's/its/ - Should the restrictions on arrays be listed here? In Christ, Steven Watanabe

Antony Polukhin

4 Oct 4 Oct

5:35 p.m.

New subject: [PFR] Review part 1 - documentation

On Sat, Oct 3, 2020, 14:22 Steven Watanabe via Boost wrote:

...

AMDG

...

- Should the restrictions on arrays be listed here?

Many thanks! Fixed all the noted issues, listed more restrictions, regenerated the docs. In Christ,

...

Steven Watanabe

Maximilian Riemensberger

7 Oct 7 Oct

5:04 p.m.

This is a short review from a user perspective. I think the precise reflection part of the library has the potential to be very useful albeit for a limited set of problems. However, for those problems it provides a significant value in my opinion. I used the library to streamline a custom protocol whose messages where already defined via aggregate types, in particular, serialization and deserialization. It turned out to be really nice to use and it let me write much less and much cleaner code. So I vote ACCEPT for the precise reflection part. I cannot see an attractive use-case, definitely not in any code I worked with. So I don't really care to much about whether its' part of the library or not. There are two things that I find really suboptimal: - Providing global operators for all aggregate types regardless of whether I want it or not is dangerous. For example, including the global_ops header in another header potentially changes the behavior of half the aggregates in a code base. I would recommend against providing this header. - The documentation could use some improvements. There are examples around printing. The examples for the impatient are a bit too impatient for my taste. A brief description (just a sentence or so) of what each snippet tries to accomplish would help a great deal for any first time user. - The Tutorial discusses flat reflection and the difference between flat and precise. If I mentally remove the flat stuff not much is left. Well printing an aggregate. I think a couple of motivating and interesting examples here that show most of the precise reflection abilities would help the potential users to grasp what the library can do how they could apply it to their concrete use cases. - The reference is somewhat difficult to navigate and somewhat hard to read by putting the links just inside the header code. I personally find the boost::asio/beast or the mp11 style references easier to work with. - Also as other reviewers have mentioned already, it's not quite clear which range of types the library can work with. A definition of a concept/requirement that specifies that (again see the asio named requirements) would certainly help. Providing a type function that checks the requirements would be a great thing as well. I spent around 5 hours playing with the library, reading the docs. I did not look into the implementation. I believe it is *magic* as the old library name suggests. And I'm certainly not qualified to judge or implement such compile-time magic. Thanks sharing the library and trying to get it into boost. I will definitely use it. Max On 9/27/20 8:16 PM, Benedek Thaler via Boost wrote:

...

The Boost formal review of the FPR (Flat Precise Reflection, ex Magic Get, ex PODs Flat Reflection) starts Today, taking place from September 28, 2020 to October 7, 2020.

The library is authored by Antony Polukhin, author of Boost.DLL, Stacktrace, Type Index libraries.

Documentation: http://apolukhin.github.io/magic_get/ Source: https://github.com/apolukhin/magic_get

The library is meant for accessing structure elements by index and providing other std::tuple like methods for user defined types without any macro or boilerplate code.

The FPR documentation summarizes the out-of-the-box added functionality for aggregate initializable structures:

- comparison operators - heterogeneous comparators - hash - stream operators - access to members by index - member reflections - methods for cooperation with std::tuple - methods to visit each field of the structure

If the description isn't immediately obvious for you, here's a motivating example:

// requires: C++14 #include <iostream> #include <string> #include "boost/pfr/precise.hpp"

struct some_person { std::string name; unsigned birth_year; };

int main() { some_person val{"Edgar Allan Poe", 1809};

std::cout << boost::pfr::get<0>(val) // No macro! << " was born in " << boost::pfr::get<1>(val); // Works with any aggregate initializables! }

Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including FPR as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).

Some other questions you might want to consider answering:

- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?

More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html

Thank you for your effort in the Boost community.

Benedek - review manager of the FPR library

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Steven Watanabe

11:06 p.m.

New subject: [PFR] Review part 2

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

Antony Polukhin

16 Oct 16 Oct

10:15 a.m.

New subject: [PFR] Review part 2

чт, 8 окт. 2020 г. в 02:07, Steven Watanabe via Boost :

...

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

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

1502

Age (days ago)

1521

Last active (days ago)

List overview

Download

12 comments

8 participants

participants (8)

Andrey Semashev
Antony Polukhin
Benedek Thaler
Hadriel Kaplan
Maximilian Riemensberger
Richard Hodges
Steven Watanabe
Vinnie Falco