[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'd also like to see some tests in Misc that confirm the attribute is being attached to the appropriate AST nodes for declarations, statements, and at namespace scope.



================
Comment at: include/clang/Basic/Attr.td:1527
+def Suppress : StmtAttr {
+  let Spellings = [CXX11<"clang", "suppress">, CXX11<"gsl", "suppress">];
+  let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
----------------
Given that this is only intended to be used with the C++ Core Guidelines, I think we should drop the `clang::suppress` spelling for this for right now. If we decide to expand the scope of this attribute to include more suppression scenarios in the future, we can always bring it back.


================
Comment at: include/clang/Basic/AttrDocs.td:2730
+The ``[[clang::suppress]]`` and ``[[gsl::suppress]]`` attributes can be used
+to suppress specific clang-tidy warnings. The ``[[gsl::suppress]]`` is an
+alias of ``[[clang::suppress]]`` which is intended to be used for suppressing
----------------
missing the word "attribute" before "is".


================
Comment at: include/clang/Basic/AttrDocs.td:2733
+rules of the C++ Core Guidelines_ in a portable way. The attributes can be
+attached to declarations, statements, and on namespace scope.
+
----------------
s/on/at.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3851
+
+    Rules.push_back(RuleName);
+  }
----------------
aaron.ballman wrote:
> Should we diagnose if asked to suppress a diagnostic that we don't support? e.g., someone does `[[clang::suppress("this does not exist")]]`
> 
> On the one hand, we want to be forwards-compatible, but on the other hand, I can easily see typos in long string literals. So perhaps we may want to have a diagnostic based on some heuristic. e.g., we diagnose if the spelling is slightly off and we have viable options which we can use typo correction to issue a fixit, or if it's way off base, we silently eat it under the assumption it's a forwards compatibility thing?
I think this needs a FIXME comment. We should probably warn the user if the rule name is unknown, but that involves a tricky layering issue since clang-tidy checks aren't trivial to expose to the Sema layer. 


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4085
+    StringRef RuleName;
+    SourceLocation LiteralLoc;
+
----------------
You can get rid of `LiteralLock` and just pass `nullptr`.


================
Comment at: lib/Sema/SemaStmtAttr.cpp:72
+                    A.getAttributeSpellingListIndex());
+  return ::new (S.Context) auto(Attr);
+}
----------------
aaron.ballman wrote:
> Please construct this directly instead of relying on a copy constructor. Also, same comments about typos apply here as above.
Same comments about the typo FIXME applies here as well. Also, you can get rid of `LiteralLock` and just pass `nullptr`.


https://reviews.llvm.org/D24886