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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: erichkeane.
aaron.ballman added a comment.

Thank you for working on this!

I had some comments, but I ran out of time before I could complete my review.



================
Comment at: clang/include/clang/Basic/Attr.td:1797
 
+def NoUniqueAddressMSVC : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
+  let Spellings = [CXX11<"msvc", "no_unique_address", 201803>];
----------------
Hmmm, it would sure be nice if we could combine this attribute with `NoUniqueAddress` above and just have an accessor for whether it's the microsoft version or not... but I think the `TargetSpecificAttr` bit prevents us from doing that. CC @erichkeane in case he's got ideas.

(No changes needed currently, mostly just a cleanup question.)


================
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>;
----------------
I was a bit shocked to learn that this really is the correct return value for `__has_cpp_attribute`: https://godbolt.org/z/MW9q1hPEh


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1410
+  For Windows targets, ``[[no_unique_address]]`` is ignored, but there is a
+  similar attribute spelled ``[[msvc::no_unique_address]]``.
   }];
----------------
"similar" makes me wonder "so how is it different?" which the docs don't answer. I think this could be stated as:
```
For Windows targets, ``[[no_unique_address]]`` is ignored; use ``[[msvc::no_unique_address]]`` instead.
```
WDYT?


================
Comment at: clang/lib/AST/Decl.cpp:4509
+  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft() && CXXRD->isUnion()) {
+    // Unions containing structs aren't zero sized?
+    for (const FieldDecl *Field : CXXRD->fields())
----------------
Hmmm, this seems to be more like MSVC doesn't believe an empty structure or union can have zero size: https://godbolt.org/z/fj4eYvnM7


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157762