Howdy, I vote to ACCEPT PFR into boost. It provides a form of reflection in a simpler manner than other solutions thus far, and for the types it can handle it’s really slick. My only minor complaint is with streaming, as discussed below. I’d also like to thank the author for responding to issues quickly on GitHub, and for bringing it to boost! - What is your evaluation of the design? Good.(?) I'm not sure how to differentiate this question from the implementation question, for this type of library. - What is your evaluation of the implementation? I didn’t study the internals, but rather used it as a user of the library. It has some behaviors I don’t personally like, but fundamentally it does what it says as far as I can tell. My only complaint is that unsigned char and signed char (and thus uint8_t and int8_t) are streamed as their char values, and thus as their unadorned ASCII characters. Since they can be non-printable values, I consider this behavior undesirable. Even as printable characters it is confusing to see - especially if the value happens to be a numeric character or a space or linefeed or whatever. The author pointed out I can choose to _not_ use the provided streaming operator, but instead roll my own streaming operator - and I did so. But I still think it’s not what the implementation should do out-of-the-box as a Boost library. Lots of aggregate types use uint8_t/int8_t for fields, I think. For example network protocol headers, data-driven-design structures to minimize cache line usage, bit fields, etc. Many (most?) people would have to write their own streaming operator due to this issue. Also, I’m a bit concerned with forward-compatibility. The definition of what constitutes an aggregate type is changing slightly in C++20, and becoming stricter I believe; so I’m worried users might find they can’t upgrade to C++20 while still using PFR for some types. But I don’t have a C++20 compiler easily available to me, so I can’t prove this to be an issue or not. (perhaps it’s not an issue at all) - What is your evaluation of the documentation? Good. It explains what needs to be explained, in a brief but reasonable manner. I didn’t study it that closely, however. - What is your evaluation of the potential usefulness of the library? In some ways this is a tough question to answer - in others it’s easy. It’s easy because - for the aggregate types it can support - it is far simpler than any other solution I know of, and thus very useful. But it’s also difficult to answer because at least for the code base in my day job, and everywhere I have worked previously, very few class/struct types would be usable with it in practice. It is not the case that it supports every aggregate type - it cannot (currently) support ones with empty base classes, including CRTP-based empty bases. We happen to use CRTP frequently in my code base; for example boost::operators. And we have a lot of data structures that are _almost_ aggregates, except they end up having either one or more private members, or user-provided constructors, or unions, or whatever. Even types in our data-driven-design code areas, ended up not being true aggregates over time. But this is not something PFR can fix or address, as far as I’m aware, nor a reason to reject it - I’m merely pointing it out as something that reduces the applicability of such an approach. (Ironically PFR is making me consider changing some of them to be true aggregates, just so that we could use PFR - so I guess in that way it’s good? :) I presume, however, that there are plenty of code bases that have lots of true aggregates, and without empty bases - and for them it would be _extremely_ useful I would think. - Did you try to use the library? Yes. I did NOT try every possible operation that PFR makes available - I mostly only did streaming/printing, iteration, and member access by index. - With which compiler(s)? Gcc 7.3, C++17 mode. - Did you have any problems? My biggest problem was finding a C++ type in my code base that PFR could support. :) There was also the printing/streaming issue described earlier. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Just a few hours of using it. - Are you knowledgeable about the problem domain? No, not really. I have never tried implementing “reflection" without using macros that required the user to identify the type’s name and all of its members. (If one can call that “reflection”) As far as I’m concerned, PFR is black magic and if this were the 17th century I would report the author to church officials. -hadriel
pt., 2 paź 2020 o 06:59 Hadriel Kaplan via Boost <boost@lists.boost.org> napisał(a):
Howdy,
I vote to ACCEPT PFR into boost. It provides a form of reflection in a simpler manner than other solutions thus far, and for the types it can handle it’s really slick. My only minor complaint is with streaming, as discussed below.
I’d also like to thank the author for responding to issues quickly on GitHub, and for bringing it to boost!
- What is your evaluation of the design?
Good.(?) I'm not sure how to differentiate this question from the implementation question, for this type of library.
- What is your evaluation of the implementation?
I didn’t study the internals, but rather used it as a user of the library. It has some behaviors I don’t personally like, but fundamentally it does what it says as far as I can tell.
My only complaint is that unsigned char and signed char (and thus uint8_t and int8_t) are streamed as their char values, and thus as their unadorned ASCII characters. Since they can be non-printable values, I consider this behavior undesirable.
But should this complaint target PFR or C++ in general? ``` int main() { uint8_t u = 52; std::cout << u << std::endl; } ``` This program outputs "4". What is broken is that `uint8_t` is an alias on `unsigned char` but as humans we expect something different from `unsigned char` and from `uint8_t`. I think PFR makes the right choice here: stream uint8_t the same way as if a user streamed it manually with `std::cout << u`. It is consistent in replicating the error-prone behavior. I think it is better to have the error-prone behavior consistent in all places than to apply fixes at random places with dubious effect: ``` struct Encoding { char alphabetLetter; int ocurrences; }; Encoding e {'a', 112}; ``` I really expect that to be output as "a 112". Regrds, &rzej;
Even as printable characters it is confusing to see - especially if the value happens to be a numeric character or a space or linefeed or whatever.
The author pointed out I can choose to _not_ use the provided streaming operator, but instead roll my own streaming operator - and I did so. But I still think it’s not what the implementation should do out-of-the-box as a Boost library.
Lots of aggregate types use uint8_t/int8_t for fields, I think. For example network protocol headers, data-driven-design structures to minimize cache line usage, bit fields, etc. Many (most?) people would have to write their own streaming operator due to this issue.
Also, I’m a bit concerned with forward-compatibility. The definition of what constitutes an aggregate type is changing slightly in C++20, and becoming stricter I believe; so I’m worried users might find they can’t upgrade to C++20 while still using PFR for some types. But I don’t have a C++20 compiler easily available to me, so I can’t prove this to be an issue or not. (perhaps it’s not an issue at all)
- What is your evaluation of the documentation?
Good. It explains what needs to be explained, in a brief but reasonable manner. I didn’t study it that closely, however.
- What is your evaluation of the potential usefulness of the library?
In some ways this is a tough question to answer - in others it’s easy.
It’s easy because - for the aggregate types it can support - it is far simpler than any other solution I know of, and thus very useful.
But it’s also difficult to answer because at least for the code base in my day job, and everywhere I have worked previously, very few class/struct types would be usable with it in practice.
It is not the case that it supports every aggregate type - it cannot (currently) support ones with empty base classes, including CRTP-based empty bases. We happen to use CRTP frequently in my code base; for example boost::operators.
And we have a lot of data structures that are _almost_ aggregates, except they end up having either one or more private members, or user-provided constructors, or unions, or whatever. Even types in our data-driven-design code areas, ended up not being true aggregates over time.
But this is not something PFR can fix or address, as far as I’m aware, nor a reason to reject it - I’m merely pointing it out as something that reduces the applicability of such an approach. (Ironically PFR is making me consider changing some of them to be true aggregates, just so that we could use PFR - so I guess in that way it’s good? :)
I presume, however, that there are plenty of code bases that have lots of true aggregates, and without empty bases - and for them it would be _extremely_ useful I would think.
- Did you try to use the library?
Yes. I did NOT try every possible operation that PFR makes available - I mostly only did streaming/printing, iteration, and member access by index.
- With which compiler(s)?
Gcc 7.3, C++17 mode.
- Did you have any problems?
My biggest problem was finding a C++ type in my code base that PFR could support. :)
There was also the printing/streaming issue described earlier.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Just a few hours of using it.
- Are you knowledgeable about the problem domain?
No, not really. I have never tried implementing “reflection" without using macros that required the user to identify the type’s name and all of its members. (If one can call that “reflection”)
As far as I’m concerned, PFR is black magic and if this were the 17th century I would report the author to church officials.
-hadriel
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Andrzej Krzemienski wrote:
My only complaint is that unsigned char and signed char (and thus uint8_t and int8_t) are streamed as their char values, and thus as their unadorned ASCII characters. Since they can be non-printable values, I consider this behavior undesirable.
But should this complaint target PFR or C++ in general?
Iostreams in general. Outputting `unsigned char*` as a null-terminated string is especially broken. I'm sure this made sense to someone in 1995.
On Oct 2, 2020, at 6:08 AM, Andrzej Krzemienski <akrzemi1@gmail.com> wrote:
My only complaint is that unsigned char and signed char (and thus uint8_t and int8_t) are streamed as their char values, and thus as their unadorned ASCII characters. Since they can be non-printable values, I consider this behavior undesirable.
But should this complaint target PFR or C++ in general? This program outputs "4". What is broken is that `uint8_t` is an alias on `unsigned char` but as humans we expect something different from `unsigned char` and from `uint8_t`. I think PFR makes the right choice here: stream uint8_t the same way as if a user streamed it manually with `std::cout << u`. It is consistent in replicating the error-prone behavior.
Of course std::ostream in general has this problem, but we’re not talking about what it does - we’re talking instead about what PFR does. :) And by that I mean, to me, how PFR chooses to stringify char-type struct members is an implementation detail of PFR and not an API contract to the user. The public API of PFR offers a streaming operator, obviously, but as far as the user knows you could choose to use any number of ways to achieve that under-the-hood for char-types. As a user, I neither know nor care how it achieves that - I only care that the output is usable. Right now, the output isn’t usable in what I would argue is the far more common use-case. But of course I could be wrong; I’m not claiming to have crystal ball. (Also, on a side-note: why shouldn’t Boost provide such things _without_ repeating mistakes in the standard library? Can we not use hindsight and real-world results to provide something better than the standard libraries?)
I think it is better to have the error-prone behavior consistent in all places than to apply fixes at random places with dubious effect:
``` struct Encoding { char alphabetLetter; int ocurrences; };
Encoding e {'a', 112}; ``` I really expect that to be output as "a 112”.
You shouldn’t want it to, imo. One of the big benefits of using PFR is you can stream structs out for debugging/logging. It’s completely useless to stream a char as a char for debugging, because the whole point is you don’t know what’s in the char, which is why you’re debugging it. *You* happen to know (or think) the `alphabetLetter` is a printable character because you named it that and are using it for that - but really it’s just a `char`, and despite being called `char` fundamentally nothing requires them to be ASCII characters (as you well know :) But let’s assume you really do want `char` types to print ‘a’ - is that the common use-case expectation, or the less-common one? Generating non-printable output doesn’t make any sense, for what I would argue are the most common use cases. -hadriel
Hadriel Kaplan wrote:
``` struct Encoding { char alphabetLetter; int ocurrences; };
Encoding e {'a', 112}; ``` I really expect that to be output as "a 112”.
You shouldn’t want it to, imo. One of the big benefits of using PFR is you can stream structs out for debugging/logging. It’s completely useless to stream a char as a char for debugging, because the whole point is you don’t know what’s in the char, which is why you’re debugging it.
The default behavior of << for chars and strings is a bit useless not just for debugging and logging, but for pretty much any use. E.g. if you have struct X { std::string a; std::string b; }; X{ "foo bar", "" }, X{ "foo", "bar" } and X{ "", "foo bar" } will all output "foo bar" under Andzrej's preferred behavior. It's hard to think where this could be useful. For logging and debugging, we want (plain, not signed or unsigned) chars to come out as 'a' or '\x04', and for strings to come out as "str\x00ing". On whether PFR needs to do this or not, I have no opinion, but it's indeed what's needed for many use cases. (I see std::quoted in the code, so this would probably match the intent.)
On Fri, Oct 2, 2020 at 1:31 PM Hadriel Kaplan via Boost <boost@lists.boost.org> wrote:
On Oct 2, 2020, at 6:08 AM, Andrzej Krzemienski <akrzemi1@gmail.com> wrote:
My only complaint is that unsigned char and signed char (and thus uint8_t and int8_t) are streamed as their char values, and thus as their unadorned ASCII characters. Since they can be non-printable values, I consider this behavior undesirable.
But should this complaint target PFR or C++ in general? This program outputs "4". What is broken is that `uint8_t` is an alias on `unsigned char` but as humans we expect something different from `unsigned char` and from `uint8_t`. I think PFR makes the right choice here: stream uint8_t the same way as if a user streamed it manually with `std::cout << u`. It is consistent in replicating the error-prone behavior.
Of course std::ostream in general has this problem, but we’re not talking about what it does - we’re talking instead about what PFR does. :)
And by that I mean, to me, how PFR chooses to stringify char-type struct members is an implementation detail of PFR and not an API contract to the user. The public API of PFR offers a streaming operator, obviously, but as far as the user knows you could choose to use any number of ways to achieve that under-the-hood for char-types.
As a user, I neither know nor care how it achieves that - I only care that the output is usable. Right now, the output isn’t usable in what I would argue is the far more common use-case. But of course I could be wrong; I’m not claiming to have crystal ball.
(Also, on a side-note: why shouldn’t Boost provide such things _without_ repeating mistakes in the standard library? Can we not use hindsight and real-world results to provide something better than the standard libraries?)
I think it is better to have the error-prone behavior consistent in all places than to apply fixes at random places with dubious effect:
``` struct Encoding { char alphabetLetter; int ocurrences; };
Encoding e {'a', 112}; ``` I really expect that to be output as "a 112”.
You shouldn’t want it to, imo. One of the big benefits of using PFR is you can stream structs out for debugging/logging. It’s completely useless to stream a char as a char for debugging, because the whole point is you don’t know what’s in the char, which is why you’re debugging it. *You* happen to know (or think) the `alphabetLetter` is a printable character because you named it that and are using it for that - but really it’s just a `char`, and despite being called `char` fundamentally nothing requires them to be ASCII characters (as you well know :)
But let’s assume you really do want `char` types to print ‘a’ - is that the common use-case expectation, or the less-common one?
Generating non-printable output doesn’t make any sense, for what I would argue are the most common use cases.
-hadriel
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
std::ostream already gives you the tools to customize the output of types by overloading operator<<. If you're having trouble debugging char values in your program, you can write an overload to print them however you please. The advantage of this approach is that it changes the output of all of your chars, not only those contained in aggregates. My expectation as a user of the aggregate operator<< would be that it's some composition of the operator<<s of each element. And as a result, that it preserves the existing ostream customization points of its member types.
On Fri, 2 Oct 2020 at 22:48, Peter Dimov via Boost <boost@lists.boost.org> wrote:
Casey Bodley wrote:
std::ostream already gives you the tools to customize the output of types by overloading operator<<.
I don't think you can overload op<< for char.
Or any fundamental type. It might be an idea to have a CPO which the PFR lib prefers if defined, falling back to operator<< if not.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
-- Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212
participants (5)
-
Andrzej Krzemienski -
Casey Bodley -
Hadriel Kaplan -
Peter Dimov -
Richard Hodges