[PATCH] D79121: Add nomerge function attribute to clang

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2755-2756
+def warn_nomerge_attribute_ignored_in_stmt: Warning<
+  "%0 attribute is ignored because there exists no call expression inside the "
+  "statement">;
 def err_nsobject_attribute : Error<
----------------
I think this should be in the `IgnoredAttributes` group as well.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:173
+static bool hasCallExpr(Stmt *S) {
+  if (S->getStmtClass() == Stmt::CallExprClass)
+    return true;
----------------
You'll need to check that `S` is nonnull, `children()` can return null children in some cases. Also, `S` should be a `const Stmt*` and I wouldn't mind a newline before the function definition.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:175-179
+  for (Stmt *SubStmt : S->children()) {
+    if (hasCallExpr(SubStmt))
+      return true;
+  }
+  return false;
----------------
I think you could replace this with: `llvm::any_of(S->children(), hasCallExpr);`


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:184
+                               SourceRange Range) {
+  NoMergeAttr attribute(S.Context, A);
+  if (S.CheckAttrNoArgs(A))
----------------
`attribute` doesn't meet the usual naming conventions. Can probably just go with `NMA` or something lame like that.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:189
+    S.Diag(St->getBeginLoc(), diag::warn_nomerge_attribute_ignored_in_stmt)
+        << attribute.getSpelling();
+    return nullptr;
----------------
You should be able to just pass `A` directly here instead.


================
Comment at: clang/test/CodeGen/attr-nomerge.cpp:6
+
+void foo(int i) {
+  [[clang::nomerge]] bar();
----------------
I'd appreciate seeing some more complex tests showing the behavior of the attribute. As some examples:
```
// Does this work on dynamically initialized global statics?
[[clang::nomerge]] static int i = foo() + bar();

void func() {
  // Is the lambda function call operator nomerge, the contained calls within the lambda, or none of the above?
  [[clang::nomerge]] [] { foo(); bar(); }(); 

  // Do we mark the implicit function calls to begin()/end() as nomerge?
  [[clang::nomerge]] for (auto foo : some_container) {}

  // Does it apply to builtins as well?
  [[clang::nomerge]] foo(__builtin_strlen("bar"), __builtin_strlen("bar"));

  // Does it apply to the substatement of a label?
  [[clang::nomerge]] how_about_this: f(bar(), bar());

  // Does it apply across the components of a for statement?
  for (foo(); bar(); baz()) {}
}
```


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

https://reviews.llvm.org/D79121