aaron.ballman added a comment.

Can you split this out into two separate reviews? That makes it easier for the 
reviewers so we don't mix up context as well as when we do code archeology for 
changes. (I also made some quick comments as I noticed things in the file, but 
I've not done a full review yet.)



================
Comment at: clang/include/clang/AST/Decl.h:2961
 
+  /// Determins whether this field has no unique address
+  bool hasNoUniqueAddress() const;
----------------



================
Comment at: clang/include/clang/Basic/Attr.td:3490
 
+def MSConstexpr : DeclOrTypeAttr {
+  let LangOpts = [MicrosoftExt];
----------------
Type attribute? That seems surprising -- this is probably meant to be 
`InheritableAttr`?


================
Comment at: clang/include/clang/Basic/Attr.td:3497
+
+def MSNoUniqueAddress : InheritableAttr, 
TargetSpecificAttr<TargetItaniumCXXABI> {
+  let LangOpts = [MicrosoftExt];
----------------
Hmmm, very odd that the msvc no unique address attribute is only supported on 
Itanium ABI targets. Should this be `TargetMicrosoftCXXABI` instead?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3366-3368
+def warn_attribute_ignored_on_non_function :
+  Warning<"%0 attribute ignored on a non-function">,
+  InGroup<IgnoredAttributes>;
----------------
You shouldn't need to add this diagnostic -- if the `Subjects` list is correct 
in Attr.td, we automatically diagnose this sort of thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134458

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D134458: [AST] Add ... Richard Dzenis via Phabricator via cfe-commits
    • [PATCH] D134458: [AST]... Aaron Ballman via Phabricator via cfe-commits

Reply via email to