[PATCH] D92800: [Clang] Make nomerge attribute a function attribute as well as a statement attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In general, I think this is reasonable -- there's a bit more work to be done before it's ready to land, but this is heading in the right direction.



================
Comment at: clang/include/clang/Basic/Attr.td:559
+/// A attribute is either a declaration attribute or a statement attribute.
+class DeclOrStmtAttr : Attr;
+
----------------
I think this should be an `InheritableAttr`, like `DeclOrTypeAttr`. With type/decl attributes, those are almost always for things like calling conventions or other cases where inheritance of the attribute is really likely to be the correct default. It's less clear to me that the same is true for stmt/decl attributes aside from the observation that most decl attributes are inheritable. We may need to come up with a better approach for inheritance here at some point when we find a stmt/decl attribute that should not be inheritable.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2185
 
+static void handleNoMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isFunctionOrMethod(D)) {
----------------
No need for this function, you can set `let SimpleHandler = 1;` in Attr.td -- the common attribute handler code should handle checking the subject list for you automatically.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7757
+  case ParsedAttr::AT_NoMerge:
+    handleNoMergeAttr(S, D, AL);
+    break;
----------------
Then this can become `handleSimpleAttribute<NoMergeAttr>(S, D, AL);`


================
Comment at: clang/test/CodeGen/attr-nomerge.cpp:73
+// CHECK-DAG: attributes #[[ATTR2]] = {{{.*}}nomerge{{.*}}}
+// CHECK-DAG: attributes #[[ATTR3]] = {{{.*}}nomerge{{.*}}}
----------------
Can you also add a test case to demonstrate that inheriting the attribute on a later redeclaration works? It can either be a codegen test or an AST dump test.


================
Comment at: clang/test/Sema/attr-nomerge.cpp:11
 
-  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute cannot be applied to a declaration}}
+  [[clang::nomerge]] label: bar(); // expected-warning {{'nomerge' attribute only applies to functions and methods}}
 
----------------
This diagnostic is no longer correct -- nomerge now applies to more things. You may need to teach the clang attribute emitter about stmt/decl attribute types so that it can generate a better diagnostic in `diagnoseAppertainsTo()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92800