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

From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2020-10-02 18:47:19

On Fri, Oct 2, 2020, 21:03 Richard Hodges via Boost <boost_at_[hidden]>
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/detectors.hpp
, 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<int, short>{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_impatient.html
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 :)

>