aaron.ballman 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);
----------------
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.
https://reviews.llvm.org/D24888
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
