dblaikie added inline comments.
================
Comment at: clang/lib/AST/Decl.cpp:4523-4524
bool FieldDecl::isPotentiallyOverlapping() const {
- return hasAttr<NoUniqueAddressAttr>() && getType()->getAsCXXRecordDecl();
+ return (hasAttr<NoUniqueAddressAttr>() ||
+ hasAttr<NoUniqueAddressMSVCAttr>()) &&
+ getType()->getAsCXXRecordDecl();
----------------
akhuang wrote:
> aaron.ballman wrote:
> > akhuang wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > Having to check both of these in several places seems problematic -
> > > > > can we wrap that up somewhere? (or, maybe ideally, is there a way for
> > > > > `msvc::no_unique_address` to map to the actual NoUniqueAddressAttr as
> > > > > a different spelling of the same thing?)
> > > > This was why I was hoping we could merge the two in Attr.td, but I'm
> > > > not certain that will be easy.
> > > What does merging the two in Attr.td mean? Could we just put the two
> > > spellings in one attribute, or would that make it impossible for clang-cl
> > > to ignore the [[no_unique_address]] spelling
> > We can have multiple syntactic spellings for the same semantic attribute
> > (e.g., `__attribute__((foo))` and `__attribute__((bar))` can both map to a
> > single `FooBarAttr` AST node), and we have "accessors" on the AST node that
> > let you tell which spelling was used:
> > https://github.com/llvm/llvm-project/blob/90ecadde62f30275c35fdf7928e3477a41691d21/clang/include/clang/Basic/Attr.td#L4095
> >
> > The suggestion Erich and I are thinking of is:
> >
> > 1) Add the additional spelling to `NoUniqueAddress`.
> > 2) Add accessors to differentiate the spellings.
> > 3) Remove the `TargetSpecificAttr` from `NoUniqueAddress`, manually
> > implement those checks in an attribute handler in SemaDeclAttr.cpp.
> >
> > Then, anywhere you care about the attribute in general, you can look for
> > `isa<NoUniqueAddress>`, and anywhere you care about which spelling, you can
> > use `cast<NoUniqueAddress>(A)->isMSVC()` (or whatever you name the
> > accessors).
> Thanks! This patch should implement this, and so far, I don't think there are
> any places outside of SemaDeclAttr where we have to differentiate the two.
(awesome - glad to see this was so tidy to do/that the generic attribute
handling stuff mostly covers it)
================
Comment at: clang/lib/AST/Decl.cpp:4510
+ if (Ctx.getTargetInfo().getCXXABI().isMicrosoft() &&
+ llvm::any_of(CXXRD->fields(), [&](const FieldDecl *Field) {
+ return Field->getType()->getAs<RecordType>();
----------------
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3096-3098
+ if (!IsOverlappingEmptyField) {
+ DataSize = std::max(DataSize, FieldOffset + Info.Size);
+ }
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157762/new/
https://reviews.llvm.org/D157762
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
