mgehre added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp:25
@@ -23,2 +24,3 @@
- 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:
> 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)
================
Comment at: clang-tidy/cppcoreguidelines/Suppression.h:59
@@ +58,3 @@
+ anyOf(hasAncestor(attributedStmt(hasSuppressAttr(Rules))),
+ hasAncestor(decl(hasAttrs(), hasSuppressAttr(Rules)))))
+ .matches(Node, Finder, Builder);
----------------
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.
https://reviews.llvm.org/D24888
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
