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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On Sun, Sep 27, 2020 at 11:17 AM Benedek Thaler via Boost
<[hidden email]> wrote:
> ...

Should the announcement also go to boost-users?

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On Mon, Sep 28, 2020, 19:12 Andrey Semashev via Boost <[hidden email]>
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.

>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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 <
[hidden email]> 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
[hidden email]
office: +442032898513
home: +376841522
mobile: +376380212

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list


> On Oct 2, 2020, at 2:02 PM, Richard Hodges via Boost <[hidden email]> 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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Fri, Oct 2, 2020, 21:03 Richard Hodges via Boost <[hidden email]>
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 :)

>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [PFR] Review part 1 - documentation

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [PFR] Review part 1 - documentation

Boost - Dev mailing list
On Sat, Oct 3, 2020, 14:22 Steven Watanabe via Boost <[hidden email]>
wrote:

> AMDG
>
<A lot of issues>

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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
пт, 2 окт. 2020 г. в 21:03, Richard Hodges via Boost <[hidden email]>:
<...>

> *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.tutorial.custom_printing_of_aggregates


> *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<char,
std::char_traits<char> >) && 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [PFR] Review part 2

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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 <boost/pfr/precise/ops.hpp>

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [PFR] Review part 2

Boost - Dev mailing list
чт, 8 окт. 2020 г. в 02:07, Steven Watanabe via Boost <[hidden email]>:
>
> 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 <boost/pfr/precise/ops.hpp>
>
> 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

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