[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

[Amy Huang via Phabricator via cfe-commits]

akhuang marked an inline comment as done.
akhuang 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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157762