[PFR][review] Andrzej's review

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

[PFR][review] Andrzej's review

Boost - Dev mailing list
Hi Everyone,
This is my partial review of PFR library. I have read the docs, watched
Antony's talk, played with some toy examples, and had a brief look at the
implementation of pfr::tuple_size. I spent like 14 hours in total on this.
I have studied the subject of tuple-like access to class objects a while
back, which resulted in this proposal:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3326.pdf.

I like the library, I think it has the potential to address a small but a
well understood demand.
While I would like to see it included in Boost, I believe it should not be
accepted just yet, in its current shape. I disagree with parts of the
design (injecting the interface of user types) as explained below. Missing
documentation parts (I mean the concept "Baseless Aggregate") is also an
issue for me. First, it might be an indication of a flaw in the design.
Second, it might cover some bugs in the implementation. Third, making
another review with updated docs might attract more reviews and may make
people appreciate the library more.

Alternatively, I could have called it a conditional acceptance with
everything listed below as conditions. Since reviewers are not casting
votes but give recommendations instead, the Review Manager may want to take
this into account.

I think that this type of the library is mostly about the documentation and
design (like, the scope). The implementation will necessarily be a number
of hacks to provide the magic. Since I question parts of the design, I
didn't find it necessary to review the implementation in detail.

The following list may appear long, and look like a long list of
complaints, so I need to make this clear up front. I really like the
library, and I intend to use it next time I have a need for the tuple-like
access to PODs, even download it from GitHub. I recommend the rejection
because I see the second review as an opportunity for the library to become
even better.


DOCUMENTATION

Documentation does explain how to use the library, and after a while it
becomes easy to use every function, at least the functions that I tried.

But it is missing some important parts, therefore it confuses the potential
users as to what its scope is, and gives an (false) impression that it is
defective. (Some of the below points may not hold anymore, as I have
noticed the documentation has changed during the review period.)

1. The library name is misleading. It uses the name "reflection" but it
doesn't really implement reflection. A reflection would be a way of
querying the system for different properties of declarations in the
program. This is not the case here.

2. The intro page misses the opportunity to present the scope of the
library, its power, and the minimal example of how to use it and how it
makes the life of the programmer easier. It is a very good library, and it
would be a great loss if it was overlooked due to not being advertised
effectively. I volunteer to provide the content of the initial page that I
believe would meet the criteria.

3. The library buds on a basic concept of "Base-less aggregate". It must
document it up front. And if possible try to statically detect if the types
satisfy it to the extent that this is doable. After reading the docs, the
user has to have a clear picture which types will and which won't work with
the library.

4. It should contain a couple of motivating use cases, both for flat and
precise representation. It is not enough to show how they work. The user
must also see why they would be useful for anyone. Some users immediately
see that this library addresses their use, others are confused, and cannot
see any purpose in having it.

5. One use case for flat reflection is when a couple of aggregates have a
common subset:

```
struct Person {
  std::string firstName;
  std::string lastName;
  int age;
};

struct Employee {
  Person person;
  double salary;
};

struct Prisoner {
  Person person
  int cellNumber;
};

std::set<Person, boost::pfr::flat_less<>>;
std::set<Employee , boost::pfr::flat_less<>>;
std::set<Prisoner , boost::pfr::flat_less<>>;
```

Just saying, "use flat variants when you want to flatten your aggregate" is
not enough to make the user see the capabilities of this library. I think
that this use case should be documented.

6. I believe that individual operations are underspecified. For instance:
i. pfr::write (http://apolukhin.github.io/magic_get/boost/pfr/write.html)
has a section "Description", but it doesn't say what the function is doing.
To some extent, we can guess that from the signature but some important
questions remain unanswered: What format of the serialized output does the
library guarantee? Does it guarantee any format at all? Do you leave
yourself a freedom to change it in the future without notice? Can users
customize it? If so, where is it specified?  (This would be a no-problem if
the library didn't provide stream insertion operations.)

ii. The library says it can inject relational operators (and  others) but
it doesn't specify what their semantics are.

7. The means of specifying the semantics of the functions could be improved
(i.e., made shorter and more precise) if you specified
prf::structure_to_tuple (and pfr::flat_structure_to_tuple) very precisely
as std::tie() on the corresponding aggregate elements, and then define the
semantics of every other function in terms of prf::structure_to_tuple.
Instead, I can see that  prf::structure_to_tuple is underspecified (
http://apolukhin.github.io/magic_get/boost/pfr/structure_to_tuple.html).
Section "Description" does not describe what the function is doing.

DESIGN

1. I am missing the concept "Base-less aggregate" that is a basic building
block of this library. I would expect it to appear in one of the front
pages of the docs, and static checks in the code that would mention this
name, so that I am immediately informed if I fail to satisfy the concept.
I realize that defining a type trait for this is impossible, but even if it
is a line of code deep inside the implementation that breaks (like,
decomposing the aggregate into a structure binding), this line should have
a comment saying something like "if compilation breaks on this line, it is
likely because your type does not satisfy the requirements of a
BaselessAggregate."

2. I appreciate functions that this library defines in namespace
boost::pfr, such as pfr::less or pfr::for_each_field or
pfr::structure_to_tuple; but I do not appreciate that this library injects
functions into global namespace or my namespace or even that it provides
operators (like <<, which are naturally part of my classes' interface) for
my types in namespace boost::pfr. I really consider this harmful, causing
subtle ADL-related bugs. If I want my types logged, I would rather expect
to be able to type:

```
// in my namespace
inline std::ostream& operator<<(std::ostream& o, MyAggregate const& a)
{
  return boost::fpr::write(o, a);
}
```

Same with relational operators: rather than library injecting them (and
potentially causing ADL problems or ODR violations, especially in C++20), I
would rather use a named PFR function where needed:

```
sort(myAggregates.begin(), myAggregates.end(), boost::fpr::less<>{});
std::set<MyAggregate, boost::fpr::less<>> map;
```

This is a bit more verbose, but:
i. Still does not require me to list members anywhere (the primary
advantage of this library).
ii. There is no possibility of getting any ADL- or ODR-related bug.

The ODR-violation bugs are really nasty. You do everything fine in your
.cpp file and you have no clue why the program desn't work as you expect.
And this is because someone else in another .cpp file has put different
declarations. And if you have 100.000 cpp files, you may not be able to
find the cause for weeks. Offering these operators encourages this type of
bugs.

3. If we eliminate the injected operators from the picture, we get a sound
interface that can be reused in the future revisions of C++, where the
internals will be able to use native C++26 reflection instead of the
current hacks. We would get the beautiful effect that the initial Boost
libraries provided: an interface that can work uniformly both with old and
modern versions of C++.

4. There is another reason why I do not like the stream insertion
operators. They just don't belong conceptually to this library. Stream
insertion is another problem domain:
i. How do you serialize type `char`: as a numeric value, or as a graphical
symbol
ii. How do you serialize a pointer? As an address it represents? Do you
make an exception for type `const char *`? Do you make an exception for
type `char32_t const*`?
iii. How do you separate the individual values? What character do you use
for grouping them?
iv. How do you allow users to control what happens for individual types?
v. What if the user is nonetheless disappointed with your design choices?
These issues go far beyond providing a tuple-like access to aggregates.
They should be addressed by a separate library built on top of PFR.

5. Docs say, "provides other std::tuple like methods for user defined types
without any macro or boilerplate code."This is not actually the case. In
order to get the operators for aggregates -- at least in one mode -- one
has to use a macro.

6. I would also say that this is inconvenient that the library, as
documented, allows me to inject all the operators or none. I cannot opt
into taking only relational operators. At least this is not documented. But
I cannot honestly complain about it, because I do not agree with PFR
synthesizing any of these operations at all.

FInally, I wanted to thank Antony for writing and sharing this library
here. I learned a great lot from it. I didn't realize that such reflection
was possible in C++17. I hope that my review does not sound discouraging.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
Andrzej Krzemienski wrote:

> 3. The library buds on a basic concept of "Base-less aggregate".

That's not really true though, is it? E.g.

#include <boost/pfr.hpp>
#include <string>
#include <iostream>

struct X
{
    std::string a;
};

struct Y
{
    std::string b;
};

struct Z: X, Y
{
    std::string c;
};

int main()
{
    std::cout << boost::pfr::tuple_size_v<Z> << std::endl;
}

compiles and prints "3". Bases are treated as members by the precise API.

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
wt., 6 paź 2020 o 13:29 Peter Dimov via Boost <[hidden email]>
napisał(a):

> Andrzej Krzemienski wrote:
>
> > 3. The library buds on a basic concept of "Base-less aggregate".
>
> That's not really true though, is it? E.g.
>
> #include <boost/pfr.hpp>
> #include <string>
> #include <iostream>
>
> struct X
> {
>     std::string a;
> };
>
> struct Y
> {
>     std::string b;
> };
>
> struct Z: X, Y
> {
>     std::string c;
> };
>
> int main()
> {
>     std::cout << boost::pfr::tuple_size_v<Z> << std::endl;
> }
>
> compiles and prints "3". Bases are treated as members by the precise API.
>

But this is only for tuple_size. If you do a for_each, the compilation
breaks:

```
Z z {{}, {}, {}};
boost::pfr::for_each_field(z, [](auto const&){});
```

This is because brace initialization of aggregates is "incompatible" with
structured binding for aggregates now in C++20.

I still acknowledge that I may have not understood the requirement that PFR
puts on types. This makes my request even stronger: I have to know what it
expects of my types to work. I expect that this would work when I switch
between C++17 and C++20. (C++20 changed the initialization of aggregates:
base is treated as yet another member. But it did not change the structured
binding.)

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
Andrzej Krzemienski wrote:
> But this is only for tuple_size. If you do a for_each, the compilation
> breaks:
>
> ```
> Z z {{}, {}, {}};
> boost::pfr::for_each_field(z, [](auto const&){});
> ```

Yes, you're right.

1>C:\boost-git\develop\boost/pfr/detail/core17_generated.hpp(57): error :
cannot decompose class type 'Z': both it and its base class 'X' have
non-static data members

That's an annoying (and unnecessary IMO) limitation of structured bindings.
Someone should write a paper to lift it (instead of just posting to the
reflector as I did.)


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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
вт, 6 окт. 2020 г. в 12:22, Andrzej Krzemienski via Boost
<[hidden email]>:
>
> Hi Everyone,
> This is my partial review of PFR library.
<...>

Many thanks!

As the reviews go I'm getting an impression that the library needs to
lose weight:
* drop the flat reflection
* drop the global operators
* drop the macro for declaring operators for type

This will make the library 2/3 smaller and simpler to understand,
along with solving the problems of macro customizations and accidental
flat_* usages.



--
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][review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
вт, 6 окт. 2020 г. в 14:42, Andrzej Krzemienski via Boost
<[hidden email]>:

>
> wt., 6 paź 2020 o 13:29 Peter Dimov via Boost <[hidden email]>
> napisał(a):
>
> > Andrzej Krzemienski wrote:
> >
> > > 3. The library buds on a basic concept of "Base-less aggregate".
> >
> > That's not really true though, is it? E.g.
> >
> > #include <boost/pfr.hpp>
> > #include <string>
> > #include <iostream>
> >
> > struct X
> > {
> >     std::string a;
> > };
> >
> > struct Y
> > {
> >     std::string b;
> > };
> >
> > struct Z: X, Y
> > {
> >     std::string c;
> > };
> >
> > int main()
> > {
> >     std::cout << boost::pfr::tuple_size_v<Z> << std::endl;
> > }
> >
> > compiles and prints "3". Bases are treated as members by the precise API.
> >
>
> But this is only for tuple_size. If you do a for_each, the compilation
> breaks:
>
> ```
> Z z {{}, {}, {}};
> boost::pfr::for_each_field(z, [](auto const&){});
> ```
>
> This is because brace initialization of aggregates is "incompatible" with
> structured binding for aggregates now in C++20.

I think we took the wrong turn in C++17, when allowed inheritance for
aggregates. Unfortunately that's not getting better with time :(

--
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][review] Andrzej's review

Boost - Dev mailing list
Antony Polukhin wrote:

> I think we took the wrong turn in C++17, when allowed inheritance for
> aggregates.

Nothing wrong with that IMO. Structured bindings just deliberately break
this case for no reason at all.

"Otherwise, all of E’s non-static data members shall be direct members of E
or of the same base class of E"


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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
wt., 6 paź 2020 o 19:53 Antony Polukhin <[hidden email]> napisał(a):

> вт, 6 окт. 2020 г. в 12:22, Andrzej Krzemienski via Boost
> <[hidden email]>:
> >
> > Hi Everyone,
> > This is my partial review of PFR library.
> <...>
>
> Many thanks!
>
> As the reviews go I'm getting an impression that the library needs to
> lose weight:
> * drop the flat reflection
> * drop the global operators
> * drop the macro for declaring operators for type
>
> This will make the library 2/3 smaller and simpler to understand,
> along with solving the problems of macro customizations and accidental
> flat_* usages.
>

I, on the other hand, during the review started to appreciate the "flat"
part. I realized I had a good use case for it.
At times, I found myself in the situation of having a number of aggregate
types with the common set of members:

```
struct Person {
  std::string firstName;
  std::string lastName;
  int age;
};

struct Employee {
  std::string firstName;
  std::string lastName;
  int age;
  double salary;
};

struct Prisoner {
  std::string firstName;
  std::string lastName;
  int age;
  int cellNumber;
};
```
I would immediately rewrite it to:

```
struct Person {
  std::string firstName;
  std::string lastName;
  int age;
};

struct Employee : Person {
  double salary;
};

struct Prisoner : Person {
  int cellNumber;
};
```
This (a) avoids duplication, and (b) I have the slicing do exactly what I
need: convert an Employee to a Person. Now, I am disappointed with what
C++17 did to aggregate initialization. My natural expectation would be that:

```
Employee e {"Bilbo", "Baggins", 111, 1000.00};
```
Flattens the class hierarchy and initializes each of the four members.
Apparently , the C++ committee has a different vision for it. But I have
noticed that if I changed derivation into aggregation (as I indicated
earlier), the "flat" part of FPR would do exactly what I needed.

I wonder what is your *primary* motivation for removing it:
A. Because it is not implementable in many cases?
B. Because there is not enough motivation for it, and people don't
understand what it is for?
C. Because it can be accidentally confused with the "precise" version?

If it is (A), then I guess you are right. If any other then maybe leaving
it in would be a better alternative.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
On 06.10.20 21:50, Andrzej Krzemienski via Boost wrote:

> struct Person {
>    std::string firstName;
>    std::string lastName;
>    int age;
> };
>
> struct Employee : Person {
>    double salary;
> };
>
> struct Prisoner : Person {
>    int cellNumber;
> };
> ```
> This (a) avoids duplication, and (b) I have the slicing do exactly what I
> need: convert an Employee to a Person. Now, I am disappointed with what
> C++17 did to aggregate initialization. My natural expectation would be that:
>
> ```
> Employee e {"Bilbo", "Baggins", 111, 1000.00};
> ```
> Flattens the class hierarchy and initializes each of the four members.
> Apparently , the C++ committee has a different vision for it. But I have
> noticed that if I changed derivation into aggregation (as I indicated
> earlier), the "flat" part of FPR would do exactly what I needed.

Does that example really work?  My expectation would be that flat
reflection would try to break apart the std::strings, and fail with a
compile-time error because std::string is not an aggregate.

If it does work, then flat reflection is a lot more useful than I had
initially thought!


--
Rainer Deyke ([hidden email])


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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
śr., 7 paź 2020 o 09:22 Rainer Deyke via Boost <[hidden email]>
napisał(a):

> On 06.10.20 21:50, Andrzej Krzemienski via Boost wrote:
> > struct Person {
> >    std::string firstName;
> >    std::string lastName;
> >    int age;
> > };
> >
> > struct Employee : Person {
> >    double salary;
> > };
> >
> > struct Prisoner : Person {
> >    int cellNumber;
> > };
> > ```
> > This (a) avoids duplication, and (b) I have the slicing do exactly what I
> > need: convert an Employee to a Person. Now, I am disappointed with what
> > C++17 did to aggregate initialization. My natural expectation would be
> that:
> >
> > ```
> > Employee e {"Bilbo", "Baggins", 111, 1000.00};
> > ```
> > Flattens the class hierarchy and initializes each of the four members.
> > Apparently , the C++ committee has a different vision for it. But I have
> > noticed that if I changed derivation into aggregation (as I indicated
> > earlier), the "flat" part of FPR would do exactly what I needed.
>
> Does that example really work?  My expectation would be that flat
> reflection would try to break apart the std::strings, and fail with a
> compile-time error because std::string is not an aggregate.
>
> If it does work, then flat reflection is a lot more useful than I had
> initially thought!
>

The following compiles and outputs "1 2 3 " on my GCC compiler:

```
struct A {
  int x, y;
};

struct B {
  A a;
  int z;
};

int main()
{
  B b = {{1, 2}, 3};
  boost::pfr::flat_for_each_field(b, [](auto const& field){ std::cout <<
field << " "; });
}
```

However, when I substitute strings for ints it no longer compiles. I am not
sure if this is just a bug in the implementation or an inherent limitation
of this library.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
śr., 7 paź 2020 o 09:58 Andrzej Krzemienski <[hidden email]> napisał(a):

>
>
> śr., 7 paź 2020 o 09:22 Rainer Deyke via Boost <[hidden email]>
> napisał(a):
>
>> On 06.10.20 21:50, Andrzej Krzemienski via Boost wrote:
>> > struct Person {
>> >    std::string firstName;
>> >    std::string lastName;
>> >    int age;
>> > };
>> >
>> > struct Employee : Person {
>> >    double salary;
>> > };
>> >
>> > struct Prisoner : Person {
>> >    int cellNumber;
>> > };
>> > ```
>> > This (a) avoids duplication, and (b) I have the slicing do exactly what
>> I
>> > need: convert an Employee to a Person. Now, I am disappointed with what
>> > C++17 did to aggregate initialization. My natural expectation would be
>> that:
>> >
>> > ```
>> > Employee e {"Bilbo", "Baggins", 111, 1000.00};
>> > ```
>> > Flattens the class hierarchy and initializes each of the four members.
>> > Apparently , the C++ committee has a different vision for it. But I have
>> > noticed that if I changed derivation into aggregation (as I indicated
>> > earlier), the "flat" part of FPR would do exactly what I needed.
>>
>> Does that example really work?  My expectation would be that flat
>> reflection would try to break apart the std::strings, and fail with a
>> compile-time error because std::string is not an aggregate.
>>
>> If it does work, then flat reflection is a lot more useful than I had
>> initially thought!
>>
>
> The following compiles and outputs "1 2 3 " on my GCC compiler:
>
> ```
> struct A {
>   int x, y;
> };
>
> struct B {
>   A a;
>   int z;
> };
>
> int main()
> {
>   B b = {{1, 2}, 3};
>   boost::pfr::flat_for_each_field(b, [](auto const& field){ std::cout <<
> field << " "; });
> }
> ```
>
> However, when I substitute strings for ints it no longer compiles. I am
> not sure if this is just a bug in the implementation or an inherent
> limitation of this library.
>

Oh, I know why it is doing this. In general, the library cannot know to
what level the user wants her structure to be flattened:

```
struct W {
  // you do not know what is inside
};
ostream& operator<<(ostream&, const W&);

struct A {
  W x, y;
};

struct B {
  A a;
  W z;
};

int main()
{
  B b = {{w1, w2}, w3};
  boost::pfr::flat_for_each_field(b, [](auto const& field){ std::cout <<
field << " "; });
}
```

Here, it is not clear whether you want to just print the W, or flatten it
further down if it happens to be an aggregate.

So maybe the "flat" part in PFR cannot solve my problem.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
ср, 7 окт. 2020 г. в 10:59, Andrzej Krzemienski via Boost
<[hidden email]>:

>
> śr., 7 paź 2020 o 09:22 Rainer Deyke via Boost <[hidden email]>
> napisał(a):
>
> > On 06.10.20 21:50, Andrzej Krzemienski via Boost wrote:
> > > struct Person {
> > >    std::string firstName;
> > >    std::string lastName;
> > >    int age;
> > > };
> > >
> > > struct Employee : Person {
> > >    double salary;
> > > };
> > >
> > > struct Prisoner : Person {
> > >    int cellNumber;
> > > };
> > > ```
> > > This (a) avoids duplication, and (b) I have the slicing do exactly what I
> > > need: convert an Employee to a Person. Now, I am disappointed with what
> > > C++17 did to aggregate initialization. My natural expectation would be
> > that:
> > >
> > > ```
> > > Employee e {"Bilbo", "Baggins", 111, 1000.00};
> > > ```
> > > Flattens the class hierarchy and initializes each of the four members.
> > > Apparently , the C++ committee has a different vision for it. But I have
> > > noticed that if I changed derivation into aggregation (as I indicated
> > > earlier), the "flat" part of FPR would do exactly what I needed.
> >
> > Does that example really work?  My expectation would be that flat
> > reflection would try to break apart the std::strings, and fail with a
> > compile-time error because std::string is not an aggregate.
> >
> > If it does work, then flat reflection is a lot more useful than I had
> > initially thought!
> >
>
> The following compiles and outputs "1 2 3 " on my GCC compiler:
>
> ```
> struct A {
>   int x, y;
> };
>
> struct B {
>   A a;
>   int z;
> };
>
> int main()
> {
>   B b = {{1, 2}, 3};
>   boost::pfr::flat_for_each_field(b, [](auto const& field){ std::cout <<
> field << " "; });
> }
> ```
>
> However, when I substitute strings for ints it no longer compiles. I am not
> sure if this is just a bug in the implementation or an inherent limitation
> of this library.

That's an inherent limitation of flat reflection.
Flat reflection is kind of a legacy - it's the first implemented
"reflection", and it was working only for PODs. Since early versions
of PFR new precise ways of reflection were discovered. Moreover,
precise reflection turned out to be much portable and works on more
compilers.

> I wonder what is your *primary* motivation for removing it:
> A. Because it is not implementable in many cases?
> B. Because there is not enough motivation for it, and people don't understand what it is for?
> C. Because it can be accidentally confused with the "precise" version?

It's rather 'D.' - unlike flat reflection, precise representation work
in much more cases and on more platforms (MSVC fails to do flat
reflection)

--
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][review] Andrzej's review

Boost - Dev mailing list
śr., 7 paź 2020 o 12:30 Antony Polukhin <[hidden email]> napisał(a):

> ср, 7 окт. 2020 г. в 10:59, Andrzej Krzemienski via Boost
> <[hidden email]>:
> >
> > śr., 7 paź 2020 o 09:22 Rainer Deyke via Boost <[hidden email]>
> > napisał(a):
> >
> > > On 06.10.20 21:50, Andrzej Krzemienski via Boost wrote:
> > > > struct Person {
> > > >    std::string firstName;
> > > >    std::string lastName;
> > > >    int age;
> > > > };
> > > >
> > > > struct Employee : Person {
> > > >    double salary;
> > > > };
> > > >
> > > > struct Prisoner : Person {
> > > >    int cellNumber;
> > > > };
> > > > ```
> > > > This (a) avoids duplication, and (b) I have the slicing do exactly
> what I
> > > > need: convert an Employee to a Person. Now, I am disappointed with
> what
> > > > C++17 did to aggregate initialization. My natural expectation would
> be
> > > that:
> > > >
> > > > ```
> > > > Employee e {"Bilbo", "Baggins", 111, 1000.00};
> > > > ```
> > > > Flattens the class hierarchy and initializes each of the four
> members.
> > > > Apparently , the C++ committee has a different vision for it. But I
> have
> > > > noticed that if I changed derivation into aggregation (as I indicated
> > > > earlier), the "flat" part of FPR would do exactly what I needed.
> > >
> > > Does that example really work?  My expectation would be that flat
> > > reflection would try to break apart the std::strings, and fail with a
> > > compile-time error because std::string is not an aggregate.
> > >
> > > If it does work, then flat reflection is a lot more useful than I had
> > > initially thought!
> > >
> >
> > The following compiles and outputs "1 2 3 " on my GCC compiler:
> >
> > ```
> > struct A {
> >   int x, y;
> > };
> >
> > struct B {
> >   A a;
> >   int z;
> > };
> >
> > int main()
> > {
> >   B b = {{1, 2}, 3};
> >   boost::pfr::flat_for_each_field(b, [](auto const& field){ std::cout <<
> > field << " "; });
> > }
> > ```
> >
> > However, when I substitute strings for ints it no longer compiles. I am
> not
> > sure if this is just a bug in the implementation or an inherent
> limitation
> > of this library.
>
> That's an inherent limitation of flat reflection.
> Flat reflection is kind of a legacy - it's the first implemented
> "reflection", and it was working only for PODs. Since early versions
> of PFR new precise ways of reflection were discovered. Moreover,
> precise reflection turned out to be much portable and works on more
> compilers.
>
> > I wonder what is your *primary* motivation for removing it:
> > A. Because it is not implementable in many cases?
> > B. Because there is not enough motivation for it, and people don't
> understand what it is for?
> > C. Because it can be accidentally confused with the "precise" version?
>
> It's rather 'D.' - unlike flat reflection, precise representation work
> in much more cases and on more platforms (MSVC fails to do flat
> reflection)
>

Interesting. I do not understand enough about the implementation to be
making definite statements, but based on my superficial knowledge I expect
that it should be possible to reimplement a "flat" reflection on top of
"precise" one.
1. First you call structure_to_tuple() and you get std::tuple<T1&, T2&,
..., Tn&>
2. For every Tn that is an aggregate type you apply structure_to_tuple()
recursively.
3. Then you potentially get a tuple of nested tuples and you need to
flatten it.

Maybe this is more difficult than it seems at first. You may also run into
different implementation limits.

Maybe someday when I have more free time I will try to write a prototype.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
ср, 7 окт. 2020 г. в 15:48, Andrzej Krzemienski <[hidden email]>:
> Interesting. I do not understand enough about the implementation to be making definite statements, but based on my superficial knowledge I expect that it should be possible to reimplement a "flat" reflection on top of "precise" one.
> 1. First you call structure_to_tuple() and you get std::tuple<T1&, T2&, ..., Tn&>
> 2. For every Tn that is an aggregate type you apply structure_to_tuple() recursively.
> 3. Then you potentially get a tuple of nested tuples and you need to flatten it.

Yes, that's doable. But is that really a popular feature?

Flat representation was useful, because there was no precise
representation in the early versions of the library. That was the only
way to do things.

Back to your example:

struct Person {
  std::string firstName;
  std::string lastName;
  int age;
};

struct Employee : Person {
  double salary;
};

struct Prisoner : Person {
  int cellNumber;
};

Probably composition would work better than inheritance for your needs?

struct Person {
  std::string firstName;
  std::string lastName;
  int age;
};

struct Employee {
  Person person;
  double salary;
};

struct Prisoner {
  Person person;
  int cellNumber;
};

With composition you get more flexibility:

std::ostream& operator<<(std::ostream& os, const Person& p) {
    os << "person: ";
    pfr::write(os, p);
    return os;
}


std::ostream& operator<<(std::ostream& os, const Prisoner& p) {
    using pfr::ops;
    return os << p;
}

// Outputs: {person: {"Harley ", "Quinn", 21}, 100500}
std::cout << Prinsoner{{"Harley ", "Quinn", 21}, 100500};

--
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][review] Andrzej's review

Boost - Dev mailing list
śr., 7 paź 2020 o 17:11 Antony Polukhin <[hidden email]> napisał(a):

> ср, 7 окт. 2020 г. в 15:48, Andrzej Krzemienski <[hidden email]>:
> > Interesting. I do not understand enough about the implementation to be
> making definite statements, but based on my superficial knowledge I expect
> that it should be possible to reimplement a "flat" reflection on top of
> "precise" one.
> > 1. First you call structure_to_tuple() and you get std::tuple<T1&, T2&,
> ..., Tn&>
> > 2. For every Tn that is an aggregate type you apply structure_to_tuple()
> recursively.
> > 3. Then you potentially get a tuple of nested tuples and you need to
> flatten it.
>
> Yes, that's doable. But is that really a popular feature?
>
> Flat representation was useful, because there was no precise
> representation in the early versions of the library. That was the only
> way to do things.
>
> Back to your example:
>
> struct Person {
>   std::string firstName;
>   std::string lastName;
>   int age;
> };
>
> struct Employee : Person {
>   double salary;
> };
>
> struct Prisoner : Person {
>   int cellNumber;
> };
>
> Probably composition would work better than inheritance for your needs?
>
> struct Person {
>   std::string firstName;
>   std::string lastName;
>   int age;
> };
>
> struct Employee {
>   Person person;
>   double salary;
> };
>
> struct Prisoner {
>   Person person;
>   int cellNumber;
> };
>
> With composition you get more flexibility:
>
> std::ostream& operator<<(std::ostream& os, const Person& p) {
>     os << "person: ";
>     pfr::write(os, p);
>     return os;
> }
>
>
> std::ostream& operator<<(std::ostream& os, const Prisoner& p) {
>     using pfr::ops;
>     return os << p;
> }
>
> // Outputs: {person: {"Harley ", "Quinn", 21}, 100500}
> std::cout << Prinsoner{{"Harley ", "Quinn", 21}, 100500};
>

I admit that the motivation for the flat reflection is much smaller than
for the precise one, so in the end it may not be worth the effort.

Regards,
&rzej;

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

Re: [PFR][review] Andrzej's review

Boost - Dev mailing list
Andrzej Krzemienski wrote:

> I admit that the motivation for the flat reflection is much smaller than
> for the precise one, so in the end it may not be worth the effort.

Flat reflection struck me as a legacy feature, one that exists only because
it came first, and Antony has admitted that that's indeed what it is.


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