[PATCH] D24888: [clang-tidy] Use [[clang::suppress]] with cppcoreguidelines-pro-type-reinterpret-cast

[Matthias Gehre via cfe-commits]

mgehre added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp:25
 
-  Finder->addMatcher(cxxReinterpretCastExpr().bind("cast"), this);
+  std::vector<StringRef> Rules{"type", "type.1", "cppcoreguidelines-pro-type-reinterpret-cast"};
+  Finder->addMatcher(cxxReinterpretCastExpr(unless(isSuppressed(Rules))).bind("cast"), this);
----------------
aaron.ballman wrote:
> malcolm.parsons wrote:
> > aaron.ballman wrote:
> > > mgehre wrote:
> > > > aaron.ballman wrote:
> > > > > Hmm, it seems like this is boilerplate we are going to want for every rule, and that it's pretty easy to not get this right (for instance, if you change the way the check is spelled, you have to remember to update this as well). Could this actually be handled more transparently, by gathering this information when the check is registered and exposing it to the check?
> > > > > 
> > > > > The checks would still need to use `unless(isSuppressed(Rules))` in some form, but I am thinking that it would be more convenient if we could do: `Finder->addMatcher(cxxReinterpretCastExpr(unlessSuppressed(*this)).bind("cast"), this);`
> > > > I see multiple ways on how to integrate that into clang-tidy:
> > > > 1) Add something like you proposed to each matcher of each check
> > > > 2) Change (or add overload of)
> > > > ```
> > > >  DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
> > > >                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
> > > > ```
> > > > to add a AST node as parameter and make the SourceLocation optional. Most checks anyhow
> > > > call this like diag(E->getLoc(), "...."), and then they would do diag(E, "...").
> > > > Diag then would check from the that AST node upwards to see if it finds a matching [[suppress]] attribute
> > > > 
> > > > 3) Change the RecursiveASTVistor that drives the AST Matches to prune certain matchers from the list of to-be-evaluated matchers
> > > > for all AST subtrees that are below a certain [[suppress]] attribute. (This is based on how I image that the AST matchers work)
> > > Ideally, I would like to see this attribute used to suppress Clang diagnostics as well, however. So while I think Option 2 is better suited to that future direction, it's still not ideal because all instances of diag() need to be updated to take advantage. Options 1 and 3 are basically limited to clang-tidy use.
> > > 
> > > I wonder if there's a way to modify the diagnostic builder to transparently handle this without requiring modifying all of the call sites?
> > clang-tidy reports how many warnings were suppressed by NOLINT comments.
> > I'd expect the number of warnings suppressed by [[clang::suppress]] to be included in the count.
> > Handling suppressions in the matchers or visitors would prevent this.
> As would handling the suppression transparently within the diagnostic engine itself.
If there is a way to turn a SourceLocation into a ASTNode, diag() could handle this transparently (do you know how?). I would still add an overload of diag  that takes an AST node as a performance optimization, because I imagine that going from SourceLocation to ASTNode would be a costly operation.
I can prototype that approach, if you like.


================
Comment at: clang-tidy/cppcoreguidelines/Suppression.h:59
+             anyOf(hasAncestor(attributedStmt(hasSuppressAttr(Rules))),
+                   hasAncestor(decl(hasAttrs(), hasSuppressAttr(Rules)))))
+      .matches(Node, Finder, Builder);
----------------
aaron.ballman wrote:
> mgehre wrote:
> > aaron.ballman wrote:
> > > Why is the check for `hasAttrs` required?
> > > 
> > > Also, this use of `hasAncestor()` makes this expensive to use, and that expense may be hidden from the caller. Is there a way to structure this so that we don't need to walk the entire ancestor tree?
> > hasAttr() is needed here, because inside of hasSuppressAttr(), we call getAttrs() which would assert if hasAttr() is false.
> > I cannot push hasAttr() into hasSuppressAttr(), because hasSuppressAttr() is a template getting either Decl or AttributedStmt,
> > and AttributedStmt does not have an hasAttr() method.
> I believe that `getSpecificAttr` should be resilient to no attributes being present (since it also has to handle the case there are no attributes of the specific type, so no attributes at all is simply a degenerate case), and so the check for `hasAttrs()` should be redundant.
Decl::getAttrs() will assert if called on a Decl where hasAttrs() is false, see
http://clang.llvm.org/doxygen/DeclBase_8cpp_source.html#l00741


https://reviews.llvm.org/D24888