[review][pfr] PFR review

From: Hadriel Kaplan (hadrielk_at_[hidden])
Date: 2020-10-02 04:59:02

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