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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

still on vaca, but doing a drive-by.



================
Comment at: clang/include/clang/Basic/Attr.td:1798
+def NoUniqueAddressMSVC : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
+  let Spellings = [CXX11<"msvc", "no_unique_address", 201803>];
+  let Subjects = SubjectList<[NonBitField], ErrorDiag>;
----------------
aaron.ballman wrote:
> I was a bit shocked to learn that this really is the correct return value for `__has_cpp_attribute`: https://godbolt.org/z/MW9q1hPEh
Thats... interesting....horrifying... else?


================
Comment at: clang/include/clang/Basic/Attr.td:1797
 
+def NoUniqueAddressMSVC : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
+  let Spellings = [CXX11<"msvc", "no_unique_address", 201803>];
----------------
I think I would combine this and the one above as a single attribute.  Then, only 'add' it in SemaDeclAttr.cpp when the spelling is the appropriate one for the current target.  IMO, msvc::no_unique_address should ONLY be valid in MSVC mode, else we encourage its propagation.

We don't need it to be a 'TargetSpecificAttr', just do the 'ignored' warning in the handler in SemaDeclAttr.td.

The accessor that we need is to just query which spelling.


================
Comment at: clang/lib/AST/Decl.cpp:4477
 
+bool FieldDecl::hasNoUniqueAddress() const {
+  return hasAttr<NoUniqueAddressAttr>() || hasAttr<NoUniqueAddressMSVCAttr>();
----------------
This ends up going away if we combine them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157762