[clang-tools-extra] [clang-tidy][NFC] Refactor `readability-redundant-control-flow` (PR #171639)
Victor Chernyakin via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 10 08:09:54 PST 2025
https://github.com/localspook created https://github.com/llvm/llvm-project/pull/171639
This is probably easiest to review commit-by-commit.
Besides simplifying the code, this refactor should also make it more efficient: instead of using the `hasAnySubstatement` matcher to find blocks we're interested in, which requires looking through every substatement, this PR introduces a custom `hasFinalStmt` matcher which only checks the last substatement.
>From dfb9d1a7b29d6215d5525bb538517b0158ec871c Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victor.j at outlook.com>
Date: Tue, 9 Dec 2025 04:06:38 -0800
Subject: [PATCH 1/5] Introduce `hasFinalStmt` matcher, simplifying a lot of
logic
---
.../readability/RedundantControlFlowCheck.cpp | 58 ++++++++-----------
.../readability/RedundantControlFlowCheck.h | 13 -----
2 files changed, 23 insertions(+), 48 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
index b77108c49f53c..9a8904b148448 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
@@ -8,12 +8,22 @@
#include "RedundantControlFlowCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
namespace clang::tidy::readability {
+namespace {
+
+AST_MATCHER_P(CompoundStmt, hasFinalStmt, StatementMatcher, InnerMatcher) {
+ return !Node.body_empty() &&
+ InnerMatcher.matches(*Node.body_back(), Finder, Builder);
+}
+
+} // namespace
+
static const char *const RedundantReturnDiag =
"redundant return statement at the end "
"of a function with a void return type";
@@ -29,45 +39,20 @@ static bool isLocationInMacroExpansion(const SourceManager &SM,
void RedundantControlFlowCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
functionDecl(isDefinition(), returns(voidType()),
- hasBody(compoundStmt(hasAnySubstatement(
- returnStmt(unless(has(expr())))))
- .bind("return"))),
- this);
- Finder->addMatcher(
- mapAnyOf(forStmt, cxxForRangeStmt, whileStmt, doStmt)
- .with(hasBody(compoundStmt(hasAnySubstatement(continueStmt()))
- .bind("continue"))),
+ hasBody(compoundStmt(hasFinalStmt(
+ returnStmt(unless(has(expr()))).bind("stmt"))))),
this);
+ Finder->addMatcher(mapAnyOf(forStmt, cxxForRangeStmt, whileStmt, doStmt)
+ .with(hasBody(compoundStmt(
+ hasFinalStmt(continueStmt().bind("stmt"))))),
+ this);
}
void RedundantControlFlowCheck::check(const MatchFinder::MatchResult &Result) {
- if (const auto *Return = Result.Nodes.getNodeAs<CompoundStmt>("return"))
- checkRedundantReturn(Result, Return);
- else if (const auto *Continue =
- Result.Nodes.getNodeAs<CompoundStmt>("continue"))
- checkRedundantContinue(Result, Continue);
-}
-
-void RedundantControlFlowCheck::checkRedundantReturn(
- const MatchFinder::MatchResult &Result, const CompoundStmt *Block) {
- const CompoundStmt::const_reverse_body_iterator Last = Block->body_rbegin();
- if (const auto *Return = dyn_cast<ReturnStmt>(*Last))
- issueDiagnostic(Result, Block, Return->getSourceRange(),
- RedundantReturnDiag);
-}
-
-void RedundantControlFlowCheck::checkRedundantContinue(
- const MatchFinder::MatchResult &Result, const CompoundStmt *Block) {
- const CompoundStmt::const_reverse_body_iterator Last = Block->body_rbegin();
- if (const auto *Continue = dyn_cast<ContinueStmt>(*Last))
- issueDiagnostic(Result, Block, Continue->getSourceRange(),
- RedundantContinueDiag);
-}
-
-void RedundantControlFlowCheck::issueDiagnostic(
- const MatchFinder::MatchResult &Result, const CompoundStmt *const Block,
- const SourceRange &StmtRange, const char *const Diag) {
+ const auto &RedundantStmt = *Result.Nodes.getNodeAs<Stmt>("stmt");
+ const SourceRange StmtRange = RedundantStmt.getSourceRange();
const SourceManager &SM = *Result.SourceManager;
+
if (isLocationInMacroExpansion(SM, StmtRange.getBegin()))
return;
@@ -77,7 +62,10 @@ void RedundantControlFlowCheck::issueDiagnostic(
getLangOpts(),
/*SkipTrailingWhitespaceAndNewLine=*/true));
- diag(StmtRange.getBegin(), Diag) << FixItHint::CreateRemoval(RemovedRange);
+ diag(StmtRange.getBegin(), isa<ReturnStmt>(RedundantStmt)
+ ? RedundantReturnDiag
+ : RedundantContinueDiag)
+ << FixItHint::CreateRemoval(RemovedRange);
}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.h b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.h
index fde305039d4c9..028f9948f74ab 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.h
@@ -30,19 +30,6 @@ class RedundantControlFlowCheck : public ClangTidyCheck {
std::optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}
-
-private:
- void
- checkRedundantReturn(const ast_matchers::MatchFinder::MatchResult &Result,
- const CompoundStmt *Block);
-
- void
- checkRedundantContinue(const ast_matchers::MatchFinder::MatchResult &Result,
- const CompoundStmt *Block);
-
- void issueDiagnostic(const ast_matchers::MatchFinder::MatchResult &Result,
- const CompoundStmt *Block, const SourceRange &StmtRange,
- const char *Diag);
};
} // namespace clang::tidy::readability
>From 2569a83046ca87cbc5f5bd56231e57fd64f122f6 Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victor.j at outlook.com>
Date: Tue, 9 Dec 2025 04:06:53 -0800
Subject: [PATCH 2/5] Prefer `StringRef` over `const char *`
---
.../clang-tidy/readability/RedundantControlFlowCheck.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
index 9a8904b148448..d86d17b8f667d 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
@@ -24,10 +24,10 @@ AST_MATCHER_P(CompoundStmt, hasFinalStmt, StatementMatcher, InnerMatcher) {
} // namespace
-static const char *const RedundantReturnDiag =
+static constexpr StringRef RedundantReturnDiag =
"redundant return statement at the end "
"of a function with a void return type";
-static const char *const RedundantContinueDiag =
+static constexpr StringRef RedundantContinueDiag =
"redundant continue statement at the "
"end of loop statement";
>From 6084a7d0e826e6b03ba78f4cc1df1db6648acd20 Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victor.j at outlook.com>
Date: Tue, 9 Dec 2025 04:09:02 -0800
Subject: [PATCH 3/5] `hasBody` implies `isDefinition`
---
.../clang-tidy/readability/RedundantControlFlowCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
index d86d17b8f667d..4ca41daabbed3 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
@@ -38,7 +38,7 @@ static bool isLocationInMacroExpansion(const SourceManager &SM,
void RedundantControlFlowCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- functionDecl(isDefinition(), returns(voidType()),
+ functionDecl(returns(voidType()),
hasBody(compoundStmt(hasFinalStmt(
returnStmt(unless(has(expr()))).bind("stmt"))))),
this);
>From 244de273be9d39a9e97e6fd4302d8bfd44439e5d Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victor.j at outlook.com>
Date: Tue, 9 Dec 2025 04:22:27 -0800
Subject: [PATCH 4/5] Simplify logic to ignore macros
---
.../clang-tidy/readability/RedundantControlFlowCheck.cpp | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
index 4ca41daabbed3..ec71cf32abb0e 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
@@ -31,11 +31,6 @@ static constexpr StringRef RedundantContinueDiag =
"redundant continue statement at the "
"end of loop statement";
-static bool isLocationInMacroExpansion(const SourceManager &SM,
- SourceLocation Loc) {
- return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
-}
-
void RedundantControlFlowCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
functionDecl(returns(voidType()),
@@ -53,7 +48,7 @@ void RedundantControlFlowCheck::check(const MatchFinder::MatchResult &Result) {
const SourceRange StmtRange = RedundantStmt.getSourceRange();
const SourceManager &SM = *Result.SourceManager;
- if (isLocationInMacroExpansion(SM, StmtRange.getBegin()))
+ if (StmtRange.getBegin().isMacroID())
return;
const auto RemovedRange = CharSourceRange::getCharRange(
>From 759ea6b630ff2cb3f69ccff176b9bedf1b3a5c99 Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victor.j at outlook.com>
Date: Tue, 9 Dec 2025 04:23:56 -0800
Subject: [PATCH 5/5] Variable `SM` is only used once; might as well inline it
---
.../clang-tidy/readability/RedundantControlFlowCheck.cpp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
index ec71cf32abb0e..46cbaa1f70301 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
@@ -46,15 +46,14 @@ void RedundantControlFlowCheck::registerMatchers(MatchFinder *Finder) {
void RedundantControlFlowCheck::check(const MatchFinder::MatchResult &Result) {
const auto &RedundantStmt = *Result.Nodes.getNodeAs<Stmt>("stmt");
const SourceRange StmtRange = RedundantStmt.getSourceRange();
- const SourceManager &SM = *Result.SourceManager;
if (StmtRange.getBegin().isMacroID())
return;
const auto RemovedRange = CharSourceRange::getCharRange(
StmtRange.getBegin(),
- Lexer::findLocationAfterToken(StmtRange.getEnd(), tok::semi, SM,
- getLangOpts(),
+ Lexer::findLocationAfterToken(StmtRange.getEnd(), tok::semi,
+ *Result.SourceManager, getLangOpts(),
/*SkipTrailingWhitespaceAndNewLine=*/true));
diag(StmtRange.getBegin(), isa<ReturnStmt>(RedundantStmt)
More information about the cfe-commits
mailing list