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

From: Richard Hodges (hodges.r_at_[hidden])
Date: 2020-10-02 18:02:37

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_at_[hidden]> 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_at_[hidden]
office: [phone]
home: [phone]
mobile: [phone]