Re: [review][pfr] PFR review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2020-10-02 10:08:28

pt., 2 paź 2020 o 06:59 Hadriel Kaplan via Boost <boost_at_[hidden]>
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
>