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