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