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