[pfr] Ready for informal mini review

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

[pfr] Ready for informal mini review

Boost - Dev mailing list
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!

--
Best regards,
Antony Polukhin

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
pt., 16 paź 2020 o 12:23 Antony Polukhin via Boost <[hidden email]>
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;

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Antony Polukhin wrote:
> 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!

Note that the last day for new libraries for Boost 1.75 is tomorrow
(2020-10-21). PFR has a develop submodule, but no master submodule
yet. I was waiting until you believe it is ready, and have ironed out
all the issues on develop.

If you want it included in the 1.75 release, and there are no
objections from any reviewers, I can add the master submodule today.

(It was already added to the in-progress release notes for 1.75, but
that can be removed if you don't want to release it until 1.76).

Glen

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
вт, 20 окт. 2020 г. в 17:00, Glen Fernandes via Boost <[hidden email]>:
<...>
> If you want it included in the 1.75 release, and there are no
> objections from any reviewers, I can add the master submodule today.

Yes, please do that.

--
Best regards,
Antony Polukhin

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
On Tue, Oct 20, 2020 at 10:40 AM Antony Polukhin <[hidden email]> wrote:
>
> вт, 20 окт. 2020 г. в 17:00, Glen Fernandes via Boost <[hidden email]>:
> <...>
> > If you want it included in the 1.75 release, and there are no
> > objections from any reviewers, I can add the master submodule today.
>
> Yes, please do that.
>

Done.

Glen

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
пн, 19 окт. 2020 г. в 23:44, Andrzej Krzemienski <[hidden email]>:

>
>
>
> pt., 16 paź 2020 o 12:23 Antony Polukhin via Boost <[hidden email]> 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?

I'd like to leave PFR abbreviation, but the title is a subject to
change. We have to find out a good one:
* Position-based Field Representation
* Pack of Fields Representation
* Pleasant Fields Representation
* Primitive Fields Representation
* ???

Your title seems to be the best so far.


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

Good point! Fixed


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

I've added a comparison with hand-written customizations (those are
simpler and shorter than BOOST_FUSION_ADAPT_STRUCT usages)

I'd rather leave the section where it is, because I do not plan to
extend it. All the other samples will go to the tutorial section.


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

I'm not very comfortable with the concept changing its requirements
depending on the C++ standard and compiler implementation.
'Limitation' seems to be a better word.


> Regards,
> &rzej;

Fixed docs: https://apolukhin.github.io/magic_get/index.html

--
Best regards,
Antony Polukhin

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

Re: [pfr] Ready for informal mini review

Boost - Dev mailing list
On Tue, Oct 20, 2020, 20:15 Antony Polukhin <[hidden email]> wrote:

> пн, 19 окт. 2020 г. в 23:44, Andrzej Krzemienski <[hidden email]>:
>
<...>

> > 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.
>
> I'm not very comfortable with the concept changing its requirements
> depending on the C++ standard and compiler implementation.
> 'Limitation' seems to be a better word.
>

I've just noted the PR https://github.com/boostorg/pfr/pull/58
You're right, much better that way! Many thanks

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