[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:420
+    auto *FD = IFD ? IFD->getAnonField() : dyn_cast<FieldDecl>(D);
+    if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+      continue;
----------------
yihanaa wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rsmith wrote:
> > > > > erichkeane wrote:
> > > > > > rsmith wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > Losing the unnamed bitfield is unfortunate, and I the 'dump struct' handles.  As is losing the anonymous struct/union.
> > > > > > > > 
> > > > > > > > Also, how does this handle 'record type' members?  Does the user have to know the inner type already?  If they already know that much information about the type, they wouldn't really need this, right?
> > > > > > > If `__builtin_dump_struct` includes unnamed bitfields, that's a bug in that builtin.
> > > > > > > 
> > > > > > > In general, the user function gets given the bases and members with the right types, so they can use an overload set or a template to dispatch based on that type. See the SemaCXX test for a basic example of that.
> > > > > > I thought it did?  For the use case I see `__builtin_dump_struct` having, I would think printing the existence of unnamed bitfields to be quite useful.
> > > > > > 
> > > > > > 
> > > > > Why? They're not part of the value, they're just padding.
> > > > They are lexically part of the type and are part of what makes up the 'size' of the thing. I would expect something dumping the type to be as consistent lexically as possible.
> > > Looks like `__builtin_dump_struct` currently includes them *and* prints a value (whatever garbage happens to be in those padding bits). That's obviously a bug.
> > O.o! Yeah, printing the value is nonsense.
> I think the user should be informed somehow that there is an unnamed bitfield here
It seems to me that `__builtin_dump_struct` is displaying the value of a struct object, not the representation of the object, and unnamed bitfields are not a part of the value, so should not be included. And I think it makes sense for that builtin to be value-oriented: someone using it presumably already knows whatever they want to know about the representation, to the extent that it matters, or can look it up; what they don't know is the information that varies from one instance to another. If we want a builtin that's more oriented around showing the struct's memory representation, I think we'd want quite a different output format, including offsets for fields -- and even then I don't think it makes sense to include unnamed bit-fields because they're not different from any other kind of padding in the object. If we still want to print unnamed bit-fields for some reason, presumably we should also print alignment and packing attributes and pragmas, because they too can change the padding within a struct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221