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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/Suppression.h:59
+             anyOf(hasAncestor(attributedStmt(hasSuppressAttr(Rules))),
+                   hasAncestor(decl(hasAttrs(), hasSuppressAttr(Rules)))))
+      .matches(Node, Finder, Builder);
----------------
mgehre wrote:
> 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
Ugh, that is a problem, but not one you should have to handle as part of this patch. I'll put it on my list of things to look into, thank you for being patient with me! :-)


https://reviews.llvm.org/D24888