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