[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:39
           varDecl(hasInitializer(
               callExpr(allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
                              callee(namedDecl(hasName("cast")))))
----------------
allOf is redudant


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:46
       callExpr(
-          allOf(
-              unless(isMacroID()), unless(cxxMemberCallExpr()),
-              allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
-                                                "dyn_cast", "dyn_cast_or_null"))
-                               .bind("func")),
-                    hasArgument(
-                        0,
-                        mapAnyOf(declRefExpr, cxxMemberCallExpr).bind("arg")))))
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
----------------
no need for allOf, just put those unless directly under callExpr


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:47
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
+                                                  "cast_if_present", "dyn_cast",
----------------
this also doesn't look to be needed..


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:47
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
+                                                  "cast_if_present", "dyn_cast",
----------------
PiotrZSL wrote:
> this also doesn't look to be needed..
maybe check argument count first `argumentCountIs(1)`
it could be cheaper than hasAnyName


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:57
   Finder->addMatcher(
       traverse(TK_AsIs,
                stmt(anyOf(
----------------
Let me quote You: "Rather than calling traverse, the canoncial way to handle this is by overriding the getCheckTraversalKind virtual function to return TK_IgnoreUnlessSpelledInSource"
In this case just return TK_AsIs.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:59
                stmt(anyOf(
                    ifStmt(Any), whileStmt(Any), doStmt(Condition),
                    binaryOperator(
----------------
Change name of this any, on first look I mistook this for a anything()


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:61
                    binaryOperator(
                        allOf(unless(isExpansionInFileMatching(
                                  "llvm/include/llvm/Support/Casting.h")),
----------------
no need for allOf, all those matchers are variadic...



================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:65-66
                              hasLHS(implicitCastExpr().bind("lhs")),
                              hasRHS(anyOf(implicitCastExpr(has(CallExpression)),
                                           CallExpression))))
                        .bind("and")))),
----------------
I think that `hasRHS(ignoringImpCasts(CallExpression))` should do a trick.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:96
+    HasIsPresent = FindIsaAndPresent(*Result.Context);
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("assign")) {
     SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
----------------
This is not a Decl but an Expr


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp:155-156
+        << ReplaceFunc
         << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
                                                     MatchedDecl->getEndLoc()),
                                         Replacement);
----------------
Maybe `MatchedDecl->getSourceRange ()` ?


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h:61
+private:
+  mutable std::optional<bool> HasIsPresent;
 };
----------------
no need for mutable, its used only form Check that isn't const.
Maybe better HasIsPresentCache ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131319/new/

https://reviews.llvm.org/D131319