https://github.com/zwuis updated
https://github.com/llvm/llvm-project/pull/155982
>From 426caa9f66cddd1deac23b397baf75f6809f6f52 Mon Sep 17 00:00:00 2001
From: Yanzuo Liu <zw...@outlook.com>
Date: Fri, 29 Aug 2025 15:15:24 +0800
Subject: [PATCH 1/3] Handle nested-name-specifier in
"llvm-prefer-isa-or-dyn-cast-in-conditionals"
---
.../PreferIsaOrDynCastInConditionalsCheck.cpp | 119 ++++++++----------
clang-tools-extra/docs/ReleaseNotes.rst | 13 ++
...prefer-isa-or-dyn-cast-in-conditionals.cpp | 75 +++++++++++
3 files changed, 143 insertions(+), 64 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
index 4c138bcc564d8..5e9777248896a 100644
---
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
+++
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
@@ -11,6 +11,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/Support/FormatVariadic.h"
using namespace clang::ast_matchers;
@@ -22,105 +23,95 @@ AST_MATCHER(Expr, isMacroID) { return
Node.getExprLoc().isMacroID(); }
void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
MatchFinder *Finder) {
- auto Condition = hasCondition(implicitCastExpr(has(
- callExpr(unless(isMacroID()), unless(cxxMemberCallExpr()),
- anyOf(callee(namedDecl(hasName("cast"))),
- callee(namedDecl(hasName("dyn_cast")).bind("dyn_cast"))))
- .bind("call"))));
-
- auto Any = anyOf(
- has(declStmt(containsDeclaration(
- 0, varDecl(hasInitializer(callExpr(unless(isMacroID()),
- unless(cxxMemberCallExpr()),
-
callee(namedDecl(hasName("cast"))))
- .bind("assign")))))),
- Condition);
-
- auto CallExpression =
+ auto AnyCalleeName = [](ArrayRef<StringRef> CalleeName) {
+ return allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+ callee(expr(ignoringImpCasts(
+ declRefExpr(to(namedDecl(hasAnyName(CalleeName))),
+ hasAnyTemplateArgumentLoc(anything()))
+ .bind("callee")))));
+ };
+
+ auto Condition = hasCondition(implicitCastExpr(
+ has(callExpr(AnyCalleeName({"cast", "dyn_cast"})).bind("cond"))));
+
+ auto Any =
+ anyOf(hasConditionVariableStatement(containsDeclaration(
+ 0, varDecl(hasInitializer(callExpr(AnyCalleeName({"cast"}))))
+ .bind("var"))),
+ Condition);
+
+ auto CallWithBindedArg =
callExpr(
-
- unless(isMacroID()), unless(cxxMemberCallExpr()),
- callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
"dyn_cast",
- "dyn_cast_or_null"))
- .bind("func")),
+ AnyCalleeName({"isa", "cast", "cast_or_null", "cast_if_present",
+ "dyn_cast", "dyn_cast_or_null",
+ "dyn_cast_if_present"}),
hasArgument(0, mapAnyOf(declRefExpr, cxxMemberCallExpr).bind("arg")))
.bind("rhs");
Finder->addMatcher(
- traverse(
- TK_AsIs,
- stmt(anyOf(
- ifStmt(Any), whileStmt(Any), doStmt(Condition),
- binaryOperator(unless(isExpansionInFileMatching(
- "llvm/include/llvm/Support/Casting.h")),
- hasOperatorName("&&"),
- hasLHS(implicitCastExpr().bind("lhs")),
-
hasRHS(anyOf(implicitCastExpr(has(CallExpression)),
- CallExpression)))
- .bind("and")))),
+ stmt(anyOf(ifStmt(Any), forStmt(Any), whileStmt(Any), doStmt(Condition),
+ binaryOperator(unless(isExpansionInFileMatching(
+ "llvm/include/llvm/Support/Casting.h")),
+ hasOperatorName("&&"),
+ hasLHS(implicitCastExpr().bind("lhs")),
+ hasRHS(ignoringImpCasts(CallWithBindedArg)))
+ .bind("and"))),
this);
}
void PreferIsaOrDynCastInConditionalsCheck::check(
const MatchFinder::MatchResult &Result) {
- if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("assign")) {
- SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
- SourceLocation EndLoc =
- StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+ const auto *Callee = Result.Nodes.getNodeAs<DeclRefExpr>("callee");
+ if (!Callee)
+ return;
+
+ SourceLocation StartLoc = Callee->getLocation();
+ SourceLocation EndLoc = Callee->getNameInfo().getEndLoc();
- diag(MatchedDecl->getBeginLoc(),
+ if (Result.Nodes.getNodeAs<VarDecl>("var")) {
+ diag(StartLoc,
"cast<> in conditional will assert rather than return a null pointer")
<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc),
"dyn_cast");
- } else if (const auto *MatchedDecl =
- Result.Nodes.getNodeAs<CallExpr>("call")) {
- SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
- SourceLocation EndLoc =
- StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
-
+ } else if (Result.Nodes.getNodeAs<CallExpr>("cond")) {
StringRef Message =
"cast<> in conditional will assert rather than return a null pointer";
- if (Result.Nodes.getNodeAs<NamedDecl>("dyn_cast"))
+ if (Callee->getDecl()->getName() == "dyn_cast")
Message = "return value from dyn_cast<> not used";
- diag(MatchedDecl->getBeginLoc(), Message)
+ diag(StartLoc, Message)
<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
- } else if (const auto *MatchedDecl =
- Result.Nodes.getNodeAs<BinaryOperator>("and")) {
+ } else if (Result.Nodes.getNodeAs<BinaryOperator>("and")) {
const auto *LHS = Result.Nodes.getNodeAs<ImplicitCastExpr>("lhs");
const auto *RHS = Result.Nodes.getNodeAs<CallExpr>("rhs");
const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
- const auto *Func = Result.Nodes.getNodeAs<NamedDecl>("func");
assert(LHS && "LHS is null");
assert(RHS && "RHS is null");
assert(Arg && "Arg is null");
- assert(Func && "Func is null");
- StringRef LHSString(Lexer::getSourceText(
- CharSourceRange::getTokenRange(LHS->getSourceRange()),
- *Result.SourceManager, getLangOpts()));
+ auto GetText = [&](SourceRange R) {
+ return Lexer::getSourceText(CharSourceRange::getTokenRange(R),
+ *Result.SourceManager, getLangOpts());
+ };
- StringRef ArgString(Lexer::getSourceText(
- CharSourceRange::getTokenRange(Arg->getSourceRange()),
- *Result.SourceManager, getLangOpts()));
+ StringRef LHSString = GetText(LHS->getSourceRange());
+
+ StringRef ArgString = GetText(Arg->getSourceRange());
if (ArgString != LHSString)
return;
- StringRef RHSString(Lexer::getSourceText(
- CharSourceRange::getTokenRange(RHS->getSourceRange()),
- *Result.SourceManager, getLangOpts()));
-
- std::string Replacement("isa_and_nonnull");
- Replacement += RHSString.substr(Func->getName().size());
+ std::string Replacement = llvm::formatv(
+ "{}{}{}", GetText(Callee->getQualifierLoc().getSourceRange()),
+ "isa_and_nonnull",
+ GetText(SourceRange(Callee->getLAngleLoc(), RHS->getEndLoc())));
- diag(MatchedDecl->getBeginLoc(),
+ diag(LHS->getBeginLoc(),
"isa_and_nonnull<> is preferred over an explicit test for null "
"followed by calling isa<>")
- << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
- MatchedDecl->getEndLoc()),
- Replacement);
+ << FixItHint::CreateReplacement(
+ SourceRange(LHS->getBeginLoc(), RHS->getEndLoc()), Replacement);
}
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 06641a602e28f..0829d336932c6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -244,6 +244,19 @@ Changes in existing checks
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
`IgnoreAliasing`, that allows not looking at underlying types of type
aliases.
+- Improved :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
+ <clang-tidy/checks/llvm/prefer-isa-or-dyn-cast-in-conditionals>` check:
+
+ - Fix-it handles Callees with nested-name-specifier correctly.
+
+ - ``if`` statements with init-statement (``if (auto X = ...; ...)``) are
+ handled correctly.
+
+ - ``for`` loops are supported.
+
+ - ``cast_if_present`` and ``dyn_cast_if_present`` are supported in the case
+ that ``isa_and_nonnull`` is preferred.
+
Removed checks
^^^^^^^^^^^^^^
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
index 88e4b643004fc..8683926bace11 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
@@ -9,14 +9,24 @@ struct Z {
bool baz(Y*);
};
+namespace llvm {
template <class X, class Y>
bool isa(Y *);
template <class X, class Y>
X *cast(Y *);
template <class X, class Y>
+X *cast_or_null(Y *);
+template <class X, class Y>
+X *cast_if_present(Y *);
+template <class X, class Y>
X *dyn_cast(Y *);
template <class X, class Y>
X *dyn_cast_or_null(Y *);
+template <class X, class Y>
+X *dyn_cast_if_present(Y *);
+} // namespace llvm
+
+using namespace llvm;
bool foo(Y *y, Z *z) {
if (auto x = cast<X>(y))
@@ -24,6 +34,26 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will
assert rather than return a null pointer
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
// CHECK-FIXES: if (auto x = dyn_cast<X>(y))
+ if (auto x = ::cast<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:18: warning: cast<> in conditional will
assert rather than return a null pointer
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (auto x = ::dyn_cast<X>(y))
+
+ if (auto x = llvm::cast<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: cast<> in conditional will
assert rather than return a null pointer
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (auto x = llvm::dyn_cast<X>(y))
+
+ if (auto x = ::llvm::cast<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: cast<> in conditional will
assert rather than return a null pointer
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (auto x = ::llvm::dyn_cast<X>(y))
+
+ for (; auto x = cast<X>(y); )
+ break;
+ // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+ // CHECK-FIXES: for (; auto x = dyn_cast<X>(y); )
+
while (auto x = cast<X>(y))
break;
// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
@@ -34,6 +64,16 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
// CHECK-FIXES: if (isa<X>(y))
+ if (auto x = cast<X>(y); cast<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: cast<> in conditional will
assert rather than return a null pointer
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (auto x = cast<X>(y); isa<X>(y))
+
+ for (; cast<X>(y); )
+ break;
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+ // CHECK-FIXES: for (; isa<X>(y); )
+
while (cast<X>(y))
break;
// CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
@@ -50,6 +90,11 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not
used [llvm-prefer-isa-or-dyn-cast-in-conditionals]
// CHECK-FIXES: if (isa<X>(y))
+ for (; dyn_cast<X>(y); )
+ break;
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<>
not used
+ // CHECK-FIXES: for (; isa<X>(y); )
+
while (dyn_cast<X>(y))
break;
// CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<>
not used
@@ -66,6 +111,21 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
over an explicit test for null followed by calling isa<>
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
// CHECK-FIXES: if (isa_and_nonnull<X>(y))
+ if (y && ::isa<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
over an explicit test for null followed by calling isa<>
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (::isa_and_nonnull<X>(y))
+
+ if (y && llvm::isa<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
over an explicit test for null followed by calling isa<>
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (llvm::isa_and_nonnull<X>(y))
+
+ if (y && ::llvm::isa<X>(y))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
over an explicit test for null followed by calling isa<>
[llvm-prefer-isa-or-dyn-cast-in-conditionals]
+ // CHECK-FIXES: if (::llvm::isa_and_nonnull<X>(y))
+
if (z->bar() && isa<Y>(z->bar()))
return true;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
@@ -76,6 +136,16 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
// CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+ if (z->bar() && cast_or_null<Y>(z->bar()))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+ // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+ if (z->bar() && cast_if_present<Y>(z->bar()))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+ // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
if (z->bar() && dyn_cast<Y>(z->bar()))
return true;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
@@ -86,6 +156,11 @@ bool foo(Y *y, Z *z) {
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
// CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+ if (z->bar() && dyn_cast_if_present<Y>(z->bar()))
+ return true;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+ // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
bool b = z->bar() && cast<Y>(z->bar());
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: isa_and_nonnull<> is preferred
// CHECK-FIXES: bool b = isa_and_nonnull<Y>(z->bar());
>From fe32d8a0ad42b7bc6ce4a0dfaee53bde950237d8 Mon Sep 17 00:00:00 2001
From: Yanzuo Liu <zw...@outlook.com>
Date: Sat, 30 Aug 2025 22:11:25 +0800
Subject: [PATCH 2/3] Remove empty line, add `const`, and make known string
inline
---
.../llvm/PreferIsaOrDynCastInConditionalsCheck.cpp | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
index 5e9777248896a..45a808cafa941 100644
---
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
+++
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
@@ -96,15 +96,14 @@ void PreferIsaOrDynCastInConditionalsCheck::check(
};
StringRef LHSString = GetText(LHS->getSourceRange());
-
StringRef ArgString = GetText(Arg->getSourceRange());
if (ArgString != LHSString)
return;
- std::string Replacement = llvm::formatv(
- "{}{}{}", GetText(Callee->getQualifierLoc().getSourceRange()),
- "isa_and_nonnull",
+ const std::string Replacement = llvm::formatv(
+ "{}isa_and_nonnull{}",
+ GetText(Callee->getQualifierLoc().getSourceRange()),
GetText(SourceRange(Callee->getLAngleLoc(), RHS->getEndLoc())));
diag(LHS->getBeginLoc(),
>From 52265dbc2641938ca955f279d351a9c9f1d42114 Mon Sep 17 00:00:00 2001
From: Yanzuo Liu <zw...@outlook.com>
Date: Sat, 30 Aug 2025 23:55:07 +0800
Subject: [PATCH 3/3] Use `assert` instead of branch
---
.../llvm/PreferIsaOrDynCastInConditionalsCheck.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
index 45a808cafa941..dae5f2bb4a379 100644
---
a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
+++
b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
@@ -62,8 +62,9 @@ void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
void PreferIsaOrDynCastInConditionalsCheck::check(
const MatchFinder::MatchResult &Result) {
const auto *Callee = Result.Nodes.getNodeAs<DeclRefExpr>("callee");
- if (!Callee)
- return;
+
+ // Callee should be matched if anything is matched.
+ assert(Callee && "Callee is null");
SourceLocation StartLoc = Callee->getLocation();
SourceLocation EndLoc = Callee->getNameInfo().getEndLoc();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
