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