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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
