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