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