[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa 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;
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124221