[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 inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1321
   let Documentation = [NoMergeDocs];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let SimpleHandler = 1;
----------------
Related to my comments in ClangAttrEmitter.cpp, I think you should make these changes instead.


================
Comment at: clang/test/CodeGen/attr-nomerge.cpp:73
+// CHECK-DAG: attributes #[[ATTR2]] = {{{.*}}nomerge{{.*}}}
+// CHECK-DAG: attributes #[[ATTR3]] = {{{.*}}nomerge{{.*}}}
----------------
zequanwu wrote:
> aaron.ballman wrote:
> > 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.
> Do you mean something like this? 
Close. I was thinking something like:
```
int g(int i); // No attr

void something() {
  // call g() here
}

[[clang::nomerge]] int g(int i);

void something_else() {
  // call g() here
}

int g(int i) { return i; }

void something_else_again() {
  // call g() here
}
```
So this tests that the attribute is inherited on redeclarations properly and it also tests the merging behavior when the declaration may not have yet had the attribute attached.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3438-3439
       }
+      if (DeclOrStmt)
+        DiagList.push_back("statements");
     }
----------------
I think this will do the wrong thing when the subject list has more than one entry (I think this will add `statements` to the list once for each subject). As an experiment, can you add `ObjCMethod` to the list of subjects for `NoMerge` in Attr.td? Do the resulting diagnostics list "statements" once or twice?

Thinking a bit deeper on this -- I think my original advice may have been too ambitious (we don't yet have support for automatically grabbing the names of statements for diagnostics like we do for declarations). Trying to add that as part of this patch would be a big ask, so I think a better approach is to use a custom diagnostic in Attr.td. I'll add a comment there to this effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92800