On Fri, Oct 2, 2020, 21:03 Richard Hodges via Boost
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
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 :)