Re: [pfr] Ready for informal mini review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2020-10-19 20:44:27

pt., 16 paź 2020 o 12:23 Antony Polukhin via Boost <boost_at_[hidden]>
napisał(a):

> Hi,
>
> Issues noted during the review period were fixed. Mainly:
> * global_ops.hpp is removed
> * boost::pfr::ops::operator* were changed to functions and moved to
> boost::pfr:: namespace
> * Flat reflection was purged
> * Big rewrite of docs
>
> I'd appreciate a look at the updated library
> https://github.com/boostorg/pfr and its docs
> https://apolukhin.github.io/magic_get/index.html
>
> Any comments and feedback is welcomed!
>

Thank you for doing the mini-review.
The documentation looks much cleaner, and it is much more intuitive.

I have some minor observations for the docs.

1. Now that the Flat part is gone, the name of the library PFR becomes hard
to understand. Unless you can retrofit some title to match the acronym
(like, Position-based Field Representation) maybe changing the name would
be appropriate?

2. The first example should be as short as possible, to convey the feel of
the library within as few seconds as possible. Reading command line args
and opening a file goes against this goal. Maybe:

```

#include <iostream>#include <string>
#include "boost/pfr.hpp"
namespace pfr = boost::pfr;
struct some_person {
    std::string name;
    unsigned birth_year;};
int main() {
    some_person val{"Edgar Allan Poe", 1809};

    std::cout << pfr::get<0>(val)                // No macro!
        << " was born in " << pfr::get<1>(val);  // Works with any
aggregate initializables!

    std::cout << boost::pfr::io(val);            // Ouputs: {"Edgar
Allan Poe", 1809}}

```

3. Section "Usecase example"  can be moved to a separate page with
motivating examples. Especially that you may wish to expand it. The current
comparison may not be fair. Without Boost.FPR what people do is not to put
all aggregate members to function `db::insert`, but rather to use a macro
or a specialization of `std::tuple_get` to enable index-based access to a
type. So a fair comparison would be one where `std::tuple_size` or
`BOOST_FUSION_ADAPT_STRUCT` is mentioned.

4. "simple aggregate" should not be listed as a "limitation, but instead be
defined as a concept or type requirement. Definitions of functions in PFR
should refer to it as requirements.

Regards,
&rzej;