https://github.com/vabridgers updated https://github.com/llvm/llvm-project/pull/175429
>From 64a84343c35e572138a8e69c28ed1470b2cd00bb Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 12:21:43 +0100 Subject: [PATCH 01/10] [clang-tidy] Add new check 'misc-scope-reduction' Adds a new clang-tidy check that does a scope reduction analysis, supporting SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA +C:2012 Rule 8-9. --- .../clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 2 + .../clang-tidy/misc/ScopeReductionCheck.cpp | 282 ++++++++++++++++++ .../clang-tidy/misc/ScopeReductionCheck.h | 28 ++ clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../docs/clang-tidy/checks/list.rst | 1 + .../checks/misc/scope-reduction.rst | 50 ++++ .../checkers/misc/scope-reduction.cpp | 181 +++++++++++ 8 files changed, 550 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index e34b0cf687be3..c1c3dd656b811 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -37,6 +37,7 @@ add_clang_library(clangTidyMiscModule STATIC OverrideWithDifferentVisibilityCheck.cpp PredictableRandCheck.cpp RedundantExpressionCheck.cpp + ScopeReductionCheck.cpp StaticAssertCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UnconventionalAssignOperatorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index f8550b30b9789..899c27033ba3b 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -26,6 +26,7 @@ #include "OverrideWithDifferentVisibilityCheck.h" #include "PredictableRandCheck.h" #include "RedundantExpressionCheck.h" +#include "ScopeReductionCheck.h" #include "StaticAssertCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UnconventionalAssignOperatorCheck.h" @@ -75,6 +76,7 @@ class MiscModule : public ClangTidyModule { CheckFactories.registerCheck<PredictableRandCheck>("misc-predictable-rand"); CheckFactories.registerCheck<RedundantExpressionCheck>( "misc-redundant-expression"); + CheckFactories.registerCheck<ScopeReductionCheck>("misc-scope-reduction"); CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert"); CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>( "misc-throw-by-value-catch-by-reference"); diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp new file mode 100644 index 0000000000000..51fb0598fa765 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -0,0 +1,282 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// This checker uses a 7-step algorithm to accomplish scope analysis of a +// variable and determine if it's in too large a scope. Note that the +// clang-tidy framework is aimed mainly at supporting text-manipulation, +// diagnostics, or common AST patterns. Scope reduction analysis is +// quite specialized, and there's not much support specifically for +// those steps. Perhaps someone else knows better and can help simplify +// this code in a more concrete way other than simply suggesting it can +// be simpler. +// +// The 7-step algorithm used by this checker for scope reduction analysis is: +// 1) Filter out variables declared in for-loop initializations +// - Those variables are already in optimal scope, and can be skipped +// 2) Collect variable uses +// - find all DeclRefExpr nodes that reference the variable +// 3) Build scope chains +// - for each use, find all compound statements that contain it (from +// innermost to outermost) +// 4) Find the innermost compound statement that contains all uses +// - This is the smallest scope where the variable could be declared +// 5) Find declaration scope +// - Locate the compound statement containing the variable declaration +// 6) Verify nesting +// - Ensure the usage scope is actually nested within the declaration scope +// 7) Alternate analysis - check for for-loop initialization opportunity +// - This is only run if compound stmt analysis didn't find smaller scope +// - Only check local variables, not parameters +// - Determine if all uses are within the same for-loop and suggest +// for-loop initialization +// +// The algo works by finding the smallest scope that could contain the variable +// declaration while still encompassing all it's uses. + +#include "ScopeReductionCheck.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::misc { + +static void +collectVariableUses(const Stmt *S, const VarDecl *Var, + llvm::SmallVector<const DeclRefExpr *, 8> &Uses) { + if (!S) + return; + + if (const auto *DRE = dyn_cast<DeclRefExpr>(S)) { + if (DRE->getDecl() == Var) + Uses.push_back(DRE); + } + + for (const Stmt *Child : S->children()) + collectVariableUses(Child, Var, Uses); +} + +void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(varDecl(hasLocalStorage()).bind("var"), this); +} + +void ScopeReductionCheck::check( + const ast_matchers::MatchFinder::MatchResult &Result) { + const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var"); + if (!Var) + return; + + // Step 1: Filter out variables declared in for-loop initializations + // These variables are already in their optimal scope and shouldn't be + // analyzed + auto &Parents = Result.Context->getParentMapContext(); + auto ParentNodes = Parents.getParents(DynTypedNode::create(*Var)); + + if (!ParentNodes.empty()) { + if (const auto *Parent = ParentNodes[0].get<Stmt>()) { + if (isa<DeclStmt>(Parent)) { + // Check if DeclStmt's parent is ForStmt + auto GrandParentNodes = Parents.getParents(*Parent); + if (!GrandParentNodes.empty()) { + if (const auto *GrandParent = GrandParentNodes[0].get<Stmt>()) { + if (isa<ForStmt>(GrandParent)) + return; // Skip for-loop declared variables + } + } + } + } + } + + // auto *Context = Result.Context; + auto *Function = dyn_cast<FunctionDecl>(Var->getDeclContext()); + if (!Function || !Function->hasBody()) + return; + + // Step 2: Collect all uses of this variable within the function + llvm::SmallVector<const DeclRefExpr *, 8> Uses; + collectVariableUses(Function->getBody(), Var, Uses); + + // No uses, return with no diagnostics + if (Uses.empty()) + return; + + // Step 3: For each variable use, find all compound statements that contain it + // This builds a "scope chain" from innermost to outermost for each use + const CompoundStmt *InnermostScope = nullptr; + + // For each use, find all compound statements that contain it + llvm::SmallVector<llvm::SmallVector<const CompoundStmt *, 4>, 8> + UseScopeChains; + + for (const auto *Use : Uses) { + llvm::SmallVector<const CompoundStmt *, 4> ScopeChain; + const Stmt *Current = Use; + + // Walk up the AST from this use to fins all containing compound stmts + while (Current) { + auto ParentNodes = Parents.getParents(*Current); + if (ParentNodes.empty()) + break; + + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) { + // Try to get Decl parent and continue from there + if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { + auto DeclParentNodes = Parents.getParents(*DeclParent); + if (!DeclParentNodes.empty()) + Parent = DeclParentNodes[0].get<Stmt>(); + } + if (!Parent) + break; + } + + if (const auto *CS = dyn_cast<CompoundStmt>(Parent)) + ScopeChain.push_back(CS); + + Current = Parent; + } + + if (!ScopeChain.empty()) + UseScopeChains.push_back(ScopeChain); + } + + // Step 4: Find the innermost scope that contains all uses + // This is the smallest scope where var could be declared + if (!UseScopeChains.empty()) { + // Start with first use's innermost scope + InnermostScope = UseScopeChains[0][0]; + + // For each subsequent use, find common ancestor scope + for (size_t i = 1; i < UseScopeChains.size(); ++i) { + const CompoundStmt *CommonScope = nullptr; + + // Find first scope that appears in both chains (common ancestor) + for (const auto *Scope1 : UseScopeChains[0]) { + for (const auto *Scope2 : UseScopeChains[i]) { + if (Scope1 == Scope2) { + CommonScope = Scope1; + break; + } + } + if (CommonScope) + break; + } + + if (CommonScope) + InnermostScope = CommonScope; + } + } + + // Step 5: Check if current var declaration is broader than necessary + if (InnermostScope) { + // Find the compound statement containing the variable declaration + const DynTypedNode Current = DynTypedNode::create(*Var); + const CompoundStmt *VarScope = nullptr; + + auto ParentNodes = Parents.getParents(Current); + while (!ParentNodes.empty()) { + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) + break; + + if (const auto *CS = dyn_cast<CompoundStmt>(Parent)) { + VarScope = CS; + break; + } + ParentNodes = Parents.getParents(*Parent); + } + + // Step 6: Verify that usage scope is nested within decl scope + if (VarScope && VarScope != InnermostScope) { + // Walk up from innermost usage to see if the decl scope is reached + const Stmt *CheckScope = InnermostScope; + bool IsNested = false; + + while (CheckScope) { + auto CheckParents = Parents.getParents(*CheckScope); + if (CheckParents.empty()) + break; + + const Stmt *CheckParent = CheckParents[0].get<Stmt>(); + if (CheckParent == VarScope) { + IsNested = true; + break; + } + CheckScope = CheckParent; + } + + // Only report if the usage scope is truly nested within the decl scope + if (IsNested) { + diag(Var->getLocation(), + "variable '%0' can be declared in a smaller scope") + << Var->getName(); + return; // early exit + } + } + } + + // Step 7: Alternative analysis - check for for-loop initialization + // opportunity This only runs if the compound statement analysis didn't find a + // smaller scope Only check local variables, not parameters + if (!isa<ParmVarDecl>(Var)) { + const ForStmt *CommonForLoop = nullptr; + bool AllUsesInSameForLoop = true; + + for (const auto *Use : Uses) { + const ForStmt *ContainingForLoop = nullptr; + const Stmt *Current = Use; + + // Walk up the AST to find a containing ForStmt + while (Current) { + auto ParentNodes = Parents.getParents(*Current); + if (ParentNodes.empty()) + break; + + if (const auto *FS = ParentNodes[0].get<ForStmt>()) { + ContainingForLoop = FS; + break; + } + + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) { + // Handle Decl parents like we do in the existing logic + if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { + auto DeclParentNodes = Parents.getParents(*DeclParent); + if (!DeclParentNodes.empty()) + Parent = DeclParentNodes[0].get<Stmt>(); + } + if (!Parent) + break; + } + Current = Parent; + } + + if (!ContainingForLoop) { + AllUsesInSameForLoop = false; + break; + } + + if (!CommonForLoop) { + CommonForLoop = ContainingForLoop; + } else if (CommonForLoop != ContainingForLoop) { + AllUsesInSameForLoop = false; + break; + } + } + + if (AllUsesInSameForLoop && CommonForLoop) { + diag(Var->getLocation(), + "variable '%0' can be declared in for-loop initialization") + << Var->getName(); + return; + } + } +} + +} // namespace clang::tidy::misc diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h new file mode 100644 index 0000000000000..690f58feb93f7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h @@ -0,0 +1,28 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDUCTIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H + +#include "../../clang-tidy/utils/DeclRefExprUtils.h" +#include "../ClangTidyCheck.h" +#include "clang/AST/ASTContext.h" + +namespace clang::tidy::misc { + +class ScopeReductionCheck : public ClangTidyCheck { +public: + ScopeReductionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::misc + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7747c5d0e96a7..2566b4f5ca730 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -294,6 +294,11 @@ New checks Finds virtual function overrides with different visibility than the function in the base class. +- New :doc:`misc-scope-reduction + <clang-tidy/checks/misc/scope-reduction>` check. + + Checks for opportunities to minimize scope of local variables. + - New :doc:`readability-inconsistent-ifelse-braces <clang-tidy/checks/readability/inconsistent-ifelse-braces>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 3dabf887dc2e1..248f237d9c318 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -280,6 +280,7 @@ Clang-Tidy Checks :doc:`misc-override-with-different-visibility <misc/override-with-different-visibility>`, :doc:`misc-predictable-rand <misc/predictable-rand>`, :doc:`misc-redundant-expression <misc/redundant-expression>`, "Yes" + :doc:`misc-scope-reduction <misc/scope-reduction>`, :doc:`misc-static-assert <misc/static-assert>`, "Yes" :doc:`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference>`, :doc:`misc-unconventional-assign-operator <misc/unconventional-assign-operator>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst new file mode 100644 index 0000000000000..11a85f406f5bc --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst @@ -0,0 +1,50 @@ +.. title:: clang-tidy - misc-scope-reduction + +misc-scope-reduction +=========================== + +Detects local variables in functions whose scopes can be minimized. This check +covers guidelines described by SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA +C:2012 Rule 8-9. + +Examples: + +.. code-block:: cpp + + void test_deep_nesting() { + int deep = 1; // 'deep' can be declared in a smaller scope + if (true) { + if (true) { + if (true) { + if (true) { + int result = deep * 4; + } + } + } + } + } + + void test_switch_multiple_cases(int value) { + int accumulator = 0; // 'accumulator' can be declared in a smaller scope + switch (value) { + case 1: + accumulator += 10; + break; + case 2: + accumulator += 20; + break; + } + } + + void test_for_loop_expressions() { + int i; // 'i' can be declared in the for-loop initialization + for (i = 0; i < 5; i++) { + // loop body + } + } + +References +---------- +This check corresponds to the CERT C Coding Standard rules +`DCL19-C. Minimize the scope of variables and functions +<https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152335/DCL19-C.+Minimize+the+scope+of+variables+and+functions>`_. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp new file mode 100644 index 0000000000000..ba9e0e9d3c466 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -0,0 +1,181 @@ +// RUN: %check_clang_tidy %s misc-scope-reduction %t -- -- + +// Test case 1: Variable can be moved to smaller scope (if-block) +void test_if_scope() { + int x = 42; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'x' can be declared in a smaller scope + if (true) { + int y = x + 1; + } +} + +// Test case 2: Variable used across multiple scopes - should NOT warn +int test_multiple_scopes(int v) { + int y = 0; // Should NOT warn - used in if-block and return + if (v) { + y = 10; + } + return y; +} + +// Test case 3: Variable can be moved to nested if-block +void test_nested_if() { + int a = 5; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'a' can be declared in a smaller scope + if (true) { + if (true) { + int b = a * 2; + } + } +} + +// Test case 4: Variable used in same scope - should NOT warn +void test_same_scope() { + int x = 10; // Should NOT warn - used in same scope + int y = x + 5; +} + +// Test case 5: Variable can be moved to while loop body +void test_while_loop() { + int counter = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'counter' can be declared in a smaller scope + while (true) { + counter++; + if (counter > 10) break; + } +} + +// Test case 6: Variable used in multiple branches of same if-statement +void test_if_branches(bool condition) { + int value = 100; // Should NOT warn - used in both branches + if (condition) { + value *= 2; + } else { + value /= 2; + } +} + +// Test case 7: Variable can be moved to for-loop body +void test_for_loop_body() { + int temp = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'temp' can be declared in a smaller scope + for (int i = 0; i < 10; i++) { + temp = i * i; + } +} + +// Test case 8: Variable used in for-loop expressions - should NOT warn (current limitation) +void test_for_loop_expressions() { + int i; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'i' can be declared in for-loop initialization + for (i = 0; i < 5; i++) { + // loop body + } +} + +// Test case 9: Variable can be moved to switch case +void test_switch_case(int value) { + int result = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'result' can be declared in a smaller scope + switch (value) { + case 1: + result = 10; + break; + default: + break; + } +} + +// Test case 10: Variable used across multiple switch cases - should NOT warn +void test_switch_multiple_cases(int value) { + int accumulator = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'accumulator' can be declared in a smaller scope + switch (value) { + case 1: + accumulator += 10; + break; + case 2: + accumulator += 20; + break; + } +} + +// Test case 11: Variable with complex initialization can be moved +void test_complex_init() { + int complex = (5 + 3) * 2; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'complex' can be declared in a smaller scope + if (true) { + int doubled = complex * 2; + } +} + +// Test case 12: Multiple variables, some can be moved, some cannot +int test_mixed_variables(bool flag) { + int movable = 10; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'movable' can be declared in a smaller scope + int unmovable = 20; // Should NOT warn - used across scopes + + if (flag) { + int local = movable + 5; + unmovable += 1; + } + + return unmovable; +} + +// Test case 13: Variable in try-catch block +void test_try_catch() { + int error_code = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'error_code' can be declared in a smaller scope + try { + error_code = 404; + } catch (...) { + // handle exception + } +} + +// Test case 14: Variable used in catch block and try block - should NOT warn +void test_try_catch_shared() { + int shared = 0; // Should NOT warn - used in both try and catch + try { + shared = 100; + } catch (...) { + shared = -1; + } +} + +// Test case 15: Deeply nested scopes +void test_deep_nesting() { + int deep = 1; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'deep' can be declared in a smaller scope + if (true) { + if (true) { + if (true) { + if (true) { + int result = deep * 4; + } + } + } + } +} + +// Test case 16: Variable declared but never used - should NOT warn (different checker's job) +void test_unused_variable() { + int unused = 42; // Should NOT warn - this checker only handles scope reduction +} + +// Test case 17: Global variable - should NOT be processed +int global_var = 100; + +// Test case 18: Static local variable - should NOT warn +void test_static_variable() { + static int static_var = 0; // Should NOT warn - static variables have different semantics + if (true) { + static_var++; + } +} + +// Test case 19: Function parameter - should NOT be processed +void test_parameter(int param) { + if (true) { + int local = param + 1; + } +} + +// Test case 20: Variable used in lambda - should NOT warn (complex case) +void test_lambda() { + int captured = 10; // Should NOT warn - used in lambda + auto lambda = [&]() { + return captured * 2; + }; + lambda(); +} >From 69347a5cbf9d5dc15d500c8e495e997a739118c8 Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 15:20:50 +0100 Subject: [PATCH 02/10] First round of code review comments * Address CI failures seen initial PR * Move all CHECK-MESSAGES to line below findings * Correct documentation format issues in rst files * Add TODO for false positive in LIT * Remove unnecessary headers, use more llvm idiom to iterate SmallVector * Annotate code with TODOs from code review (helps me) --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 9 ++--- .../clang-tidy/misc/ScopeReductionCheck.h | 1 - .../checks/misc/scope-reduction.rst | 3 +- .../checkers/misc/scope-reduction.cpp | 35 +++++++++++++------ 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index 51fb0598fa765..5034a426c43e3 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -39,14 +39,13 @@ // declaration while still encompassing all it's uses. #include "ScopeReductionCheck.h" -#include "../utils/ASTUtils.h" -#include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" using namespace clang::ast_matchers; namespace clang::tidy::misc { +// TODO: Try using utils::decl_ref_expr::allDeclRefExprs here. static void collectVariableUses(const Stmt *S, const VarDecl *Var, llvm::SmallVector<const DeclRefExpr *, 8> &Uses) { @@ -63,6 +62,8 @@ collectVariableUses(const Stmt *S, const VarDecl *Var, } void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { + // TODO: Try adding unless(hasParent(declStmt(hasParent(forStmt( to matcher + // to simplify check code. Finder->addMatcher(varDecl(hasLocalStorage()).bind("var"), this); } @@ -153,12 +154,12 @@ void ScopeReductionCheck::check( InnermostScope = UseScopeChains[0][0]; // For each subsequent use, find common ancestor scope - for (size_t i = 1; i < UseScopeChains.size(); ++i) { + for (const auto &ScopeChain : llvm::drop_begin(UseScopeChains)) { const CompoundStmt *CommonScope = nullptr; // Find first scope that appears in both chains (common ancestor) for (const auto *Scope1 : UseScopeChains[0]) { - for (const auto *Scope2 : UseScopeChains[i]) { + for (const auto *Scope2 : ScopeChain) { if (Scope1 == Scope2) { CommonScope = Scope1; break; diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h index 690f58feb93f7..ef3c898f9c95f 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h @@ -11,7 +11,6 @@ #include "../../clang-tidy/utils/DeclRefExprUtils.h" #include "../ClangTidyCheck.h" -#include "clang/AST/ASTContext.h" namespace clang::tidy::misc { diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst index 11a85f406f5bc..5c8090fe7c274 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst @@ -1,7 +1,7 @@ .. title:: clang-tidy - misc-scope-reduction misc-scope-reduction -=========================== +==================== Detects local variables in functions whose scopes can be minimized. This check covers guidelines described by SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA @@ -45,6 +45,7 @@ Examples: References ---------- + This check corresponds to the CERT C Coding Standard rules `DCL19-C. Minimize the scope of variables and functions <https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152335/DCL19-C.+Minimize+the+scope+of+variables+and+functions>`_. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp index ba9e0e9d3c466..fcc6ea2613495 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -2,7 +2,8 @@ // Test case 1: Variable can be moved to smaller scope (if-block) void test_if_scope() { - int x = 42; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'x' can be declared in a smaller scope + int x = 42; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'x' can be declared in a smaller scope if (true) { int y = x + 1; } @@ -19,7 +20,8 @@ int test_multiple_scopes(int v) { // Test case 3: Variable can be moved to nested if-block void test_nested_if() { - int a = 5; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'a' can be declared in a smaller scope + int a = 5; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' can be declared in a smaller scope if (true) { if (true) { int b = a * 2; @@ -34,8 +36,11 @@ void test_same_scope() { } // Test case 5: Variable can be moved to while loop body +// TODO: This is a false positive. Correcting this will require +// loop semantic comprehension and var lifetime analysis. void test_while_loop() { - int counter = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'counter' can be declared in a smaller scope + int counter = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'counter' can be declared in a smaller scope while (true) { counter++; if (counter > 10) break; @@ -54,7 +59,8 @@ void test_if_branches(bool condition) { // Test case 7: Variable can be moved to for-loop body void test_for_loop_body() { - int temp = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'temp' can be declared in a smaller scope + int temp = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'temp' can be declared in a smaller scope for (int i = 0; i < 10; i++) { temp = i * i; } @@ -62,7 +68,8 @@ void test_for_loop_body() { // Test case 8: Variable used in for-loop expressions - should NOT warn (current limitation) void test_for_loop_expressions() { - int i; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'i' can be declared in for-loop initialization + int i; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'i' can be declared in for-loop initialization for (i = 0; i < 5; i++) { // loop body } @@ -70,7 +77,8 @@ void test_for_loop_expressions() { // Test case 9: Variable can be moved to switch case void test_switch_case(int value) { - int result = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'result' can be declared in a smaller scope + int result = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'result' can be declared in a smaller scope switch (value) { case 1: result = 10; @@ -82,7 +90,8 @@ void test_switch_case(int value) { // Test case 10: Variable used across multiple switch cases - should NOT warn void test_switch_multiple_cases(int value) { - int accumulator = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'accumulator' can be declared in a smaller scope + int accumulator = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'accumulator' can be declared in a smaller scope switch (value) { case 1: accumulator += 10; @@ -95,7 +104,8 @@ void test_switch_multiple_cases(int value) { // Test case 11: Variable with complex initialization can be moved void test_complex_init() { - int complex = (5 + 3) * 2; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'complex' can be declared in a smaller scope + int complex = (5 + 3) * 2; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'complex' can be declared in a smaller scope if (true) { int doubled = complex * 2; } @@ -103,7 +113,8 @@ void test_complex_init() { // Test case 12: Multiple variables, some can be moved, some cannot int test_mixed_variables(bool flag) { - int movable = 10; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'movable' can be declared in a smaller scope + int movable = 10; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'movable' can be declared in a smaller scope int unmovable = 20; // Should NOT warn - used across scopes if (flag) { @@ -116,7 +127,8 @@ int test_mixed_variables(bool flag) { // Test case 13: Variable in try-catch block void test_try_catch() { - int error_code = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'error_code' can be declared in a smaller scope + int error_code = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'error_code' can be declared in a smaller scope try { error_code = 404; } catch (...) { @@ -136,7 +148,8 @@ void test_try_catch_shared() { // Test case 15: Deeply nested scopes void test_deep_nesting() { - int deep = 1; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'deep' can be declared in a smaller scope + int deep = 1; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'deep' can be declared in a smaller scope if (true) { if (true) { if (true) { >From 63c653a76be822552495101edc364b3c8c41b423 Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 16:04:07 +0100 Subject: [PATCH 03/10] Update to correct CI issues missed in last update * Remove include of DeclRefExprUtils.h * Correct spelling in header guard --- clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h index ef3c898f9c95f..15fb82cc02fb5 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h @@ -7,9 +7,8 @@ //===----------------------------------------------------------------------===// #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDUCTIONCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDUCTIONCHECK_H -#include "../../clang-tidy/utils/DeclRefExprUtils.h" #include "../ClangTidyCheck.h" namespace clang::tidy::misc { >From e76d506c6233972dc5d1e5ba9748556e51313a6a Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 16:12:04 +0100 Subject: [PATCH 04/10] Update missing parts * I guess I keep missing small things :/ * Addressed next header guard spelling. Maybe CI will pass now. --- clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h index 15fb82cc02fb5..ee5a94daaa855 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h @@ -23,4 +23,4 @@ class ScopeReductionCheck : public ClangTidyCheck { } // namespace clang::tidy::misc -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDUCTIONCHECK_H >From c652ddd0d758bc8bc8947f78492ab962a0f05a7e Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 19:17:27 +0100 Subject: [PATCH 05/10] Update review from comments * Use clang::tidy::utils::decl_ref_expr::allDeclRefExprs * Remove some useless comments * Narrow filter, find vars that have function body * Assert on var from matcher in check instead of checking for NULL * Add some TODOs * Update rst hopefully one last time for CI to pass --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 37 +++++++++---------- .../checks/misc/scope-reduction.rst | 4 +- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index 5034a426c43e3..a959b8367abac 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -40,38 +40,38 @@ #include "ScopeReductionCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "../utils/DeclRefExprUtils.h" using namespace clang::ast_matchers; namespace clang::tidy::misc { -// TODO: Try using utils::decl_ref_expr::allDeclRefExprs here. -static void -collectVariableUses(const Stmt *S, const VarDecl *Var, - llvm::SmallVector<const DeclRefExpr *, 8> &Uses) { - if (!S) +static void collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, + llvm::SmallVector<const clang::DeclRefExpr *, 8> &Uses) { + if (!S || !Var) return; - if (const auto *DRE = dyn_cast<DeclRefExpr>(S)) { - if (DRE->getDecl() == Var) - Uses.push_back(DRE); - } + llvm::SmallPtrSet<const clang::DeclRefExpr *, 16> DREs = + clang::tidy::utils::decl_ref_expr::allDeclRefExprs(*Var, *S, Var->getASTContext()); - for (const Stmt *Child : S->children()) - collectVariableUses(Child, Var, Uses); + // Copy the results into the provided SmallVector + Uses.clear(); + Uses.append(DREs.begin(), DREs.end()); } void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { // TODO: Try adding unless(hasParent(declStmt(hasParent(forStmt( to matcher // to simplify check code. - Finder->addMatcher(varDecl(hasLocalStorage()).bind("var"), this); + + // Match on varDecls that are part of a function + Finder->addMatcher(varDecl(hasLocalStorage(), hasAncestor(functionDecl())).bind("var"), this); } void ScopeReductionCheck::check( const ast_matchers::MatchFinder::MatchResult &Result) { const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var"); - if (!Var) - return; + + assert(Var); // Step 1: Filter out variables declared in for-loop initializations // These variables are already in their optimal scope and shouldn't be @@ -94,10 +94,8 @@ void ScopeReductionCheck::check( } } - // auto *Context = Result.Context; - auto *Function = dyn_cast<FunctionDecl>(Var->getDeclContext()); - if (!Function || !Function->hasBody()) - return; + const auto *Function = dyn_cast<FunctionDecl>(Var->getDeclContext()); + assert(Function); // Step 2: Collect all uses of this variable within the function llvm::SmallVector<const DeclRefExpr *, 8> Uses; @@ -217,7 +215,7 @@ void ScopeReductionCheck::check( diag(Var->getLocation(), "variable '%0' can be declared in a smaller scope") << Var->getName(); - return; // early exit + return; } } } @@ -225,6 +223,7 @@ void ScopeReductionCheck::check( // Step 7: Alternative analysis - check for for-loop initialization // opportunity This only runs if the compound statement analysis didn't find a // smaller scope Only check local variables, not parameters + // TODO: ParmVarDecls maybe excluded for all analysis. if (!isa<ParmVarDecl>(Var)) { const ForStmt *CommonForLoop = nullptr; bool AllUsesInSameForLoop = true; diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst index 5c8090fe7c274..46b3e163f83d8 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst @@ -4,8 +4,8 @@ misc-scope-reduction ==================== Detects local variables in functions whose scopes can be minimized. This check -covers guidelines described by SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA -C:2012 Rule 8-9. +covers guidelines described by SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and +MISRA C:2012 Rule 8-9. Examples: >From 834c0fb518a5b3e9e46001905f8dc6181c51c1a8 Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 19:46:09 +0100 Subject: [PATCH 06/10] More updates per review * Remove test case numbers * Pretty format last set of changes (maybe CI will pass!) * Sync Release notes and documentation per review comment --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 14 ++++--- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- .../checkers/misc/scope-reduction.cpp | 40 +++++++++---------- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index a959b8367abac..b1a462776fca0 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -39,20 +39,22 @@ // declaration while still encompassing all it's uses. #include "ScopeReductionCheck.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" #include "../utils/DeclRefExprUtils.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" using namespace clang::ast_matchers; namespace clang::tidy::misc { -static void collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, - llvm::SmallVector<const clang::DeclRefExpr *, 8> &Uses) { +static void +collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, + llvm::SmallVector<const clang::DeclRefExpr *, 8> &Uses) { if (!S || !Var) return; llvm::SmallPtrSet<const clang::DeclRefExpr *, 16> DREs = - clang::tidy::utils::decl_ref_expr::allDeclRefExprs(*Var, *S, Var->getASTContext()); + clang::tidy::utils::decl_ref_expr::allDeclRefExprs(*Var, *S, + Var->getASTContext()); // Copy the results into the provided SmallVector Uses.clear(); @@ -64,7 +66,9 @@ void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { // to simplify check code. // Match on varDecls that are part of a function - Finder->addMatcher(varDecl(hasLocalStorage(), hasAncestor(functionDecl())).bind("var"), this); + Finder->addMatcher( + varDecl(hasLocalStorage(), hasAncestor(functionDecl())).bind("var"), + this); } void ScopeReductionCheck::check( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 2566b4f5ca730..cd124b3eafaf3 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -297,7 +297,7 @@ New checks - New :doc:`misc-scope-reduction <clang-tidy/checks/misc/scope-reduction>` check. - Checks for opportunities to minimize scope of local variables. + Detects local variables in function whose scopes can be minimized. - New :doc:`readability-inconsistent-ifelse-braces <clang-tidy/checks/readability/inconsistent-ifelse-braces>` check. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp index fcc6ea2613495..dc5112e3dccb5 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -1,6 +1,6 @@ // RUN: %check_clang_tidy %s misc-scope-reduction %t -- -- -// Test case 1: Variable can be moved to smaller scope (if-block) +// Variable can be moved to smaller scope (if-block) void test_if_scope() { int x = 42; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'x' can be declared in a smaller scope @@ -9,7 +9,7 @@ void test_if_scope() { } } -// Test case 2: Variable used across multiple scopes - should NOT warn +// Variable used across multiple scopes - should NOT warn int test_multiple_scopes(int v) { int y = 0; // Should NOT warn - used in if-block and return if (v) { @@ -18,7 +18,7 @@ int test_multiple_scopes(int v) { return y; } -// Test case 3: Variable can be moved to nested if-block +// Variable can be moved to nested if-block void test_nested_if() { int a = 5; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' can be declared in a smaller scope @@ -29,13 +29,13 @@ void test_nested_if() { } } -// Test case 4: Variable used in same scope - should NOT warn +// Variable used in same scope - should NOT warn void test_same_scope() { int x = 10; // Should NOT warn - used in same scope int y = x + 5; } -// Test case 5: Variable can be moved to while loop body +// Variable can be moved to while loop body // TODO: This is a false positive. Correcting this will require // loop semantic comprehension and var lifetime analysis. void test_while_loop() { @@ -47,7 +47,7 @@ void test_while_loop() { } } -// Test case 6: Variable used in multiple branches of same if-statement +// Variable used in multiple branches of same if-statement void test_if_branches(bool condition) { int value = 100; // Should NOT warn - used in both branches if (condition) { @@ -57,7 +57,7 @@ void test_if_branches(bool condition) { } } -// Test case 7: Variable can be moved to for-loop body +// Variable can be moved to for-loop body void test_for_loop_body() { int temp = 0; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'temp' can be declared in a smaller scope @@ -66,7 +66,7 @@ void test_for_loop_body() { } } -// Test case 8: Variable used in for-loop expressions - should NOT warn (current limitation) +// Variable used in for-loop expressions - should NOT warn (current limitation) void test_for_loop_expressions() { int i; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'i' can be declared in for-loop initialization @@ -75,7 +75,7 @@ void test_for_loop_expressions() { } } -// Test case 9: Variable can be moved to switch case +// Variable can be moved to switch case void test_switch_case(int value) { int result = 0; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'result' can be declared in a smaller scope @@ -88,7 +88,7 @@ void test_switch_case(int value) { } } -// Test case 10: Variable used across multiple switch cases - should NOT warn +// Variable used across multiple switch cases - should NOT warn void test_switch_multiple_cases(int value) { int accumulator = 0; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'accumulator' can be declared in a smaller scope @@ -102,7 +102,7 @@ void test_switch_multiple_cases(int value) { } } -// Test case 11: Variable with complex initialization can be moved +// Variable with complex initialization can be moved void test_complex_init() { int complex = (5 + 3) * 2; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'complex' can be declared in a smaller scope @@ -111,7 +111,7 @@ void test_complex_init() { } } -// Test case 12: Multiple variables, some can be moved, some cannot +// Multiple variables, some can be moved, some cannot int test_mixed_variables(bool flag) { int movable = 10; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'movable' can be declared in a smaller scope @@ -125,7 +125,7 @@ int test_mixed_variables(bool flag) { return unmovable; } -// Test case 13: Variable in try-catch block +// Variable in try-catch block void test_try_catch() { int error_code = 0; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'error_code' can be declared in a smaller scope @@ -136,7 +136,7 @@ void test_try_catch() { } } -// Test case 14: Variable used in catch block and try block - should NOT warn +// Variable used in catch block and try block - should NOT warn void test_try_catch_shared() { int shared = 0; // Should NOT warn - used in both try and catch try { @@ -146,7 +146,7 @@ void test_try_catch_shared() { } } -// Test case 15: Deeply nested scopes +// Deeply nested scopes void test_deep_nesting() { int deep = 1; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'deep' can be declared in a smaller scope @@ -161,15 +161,15 @@ void test_deep_nesting() { } } -// Test case 16: Variable declared but never used - should NOT warn (different checker's job) +// Variable declared but never used - should NOT warn (different checker's job) void test_unused_variable() { int unused = 42; // Should NOT warn - this checker only handles scope reduction } -// Test case 17: Global variable - should NOT be processed +// Global variable - should NOT be processed int global_var = 100; -// Test case 18: Static local variable - should NOT warn +// Static local variable - should NOT warn void test_static_variable() { static int static_var = 0; // Should NOT warn - static variables have different semantics if (true) { @@ -177,14 +177,14 @@ void test_static_variable() { } } -// Test case 19: Function parameter - should NOT be processed +// Function parameter - should NOT be processed void test_parameter(int param) { if (true) { int local = param + 1; } } -// Test case 20: Variable used in lambda - should NOT warn (complex case) +// Variable used in lambda - should NOT warn (complex case) void test_lambda() { int captured = 10; // Should NOT warn - used in lambda auto lambda = [&]() { >From bbbb3b4beb239723b8018cf67f321b550566fbd6 Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Sun, 11 Jan 2026 22:39:19 +0100 Subject: [PATCH 07/10] Update - address more comments * Add const qualifier to address tidy warning * Add, update test cases per comments --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 2 +- .../checkers/misc/scope-reduction.cpp | 22 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index b1a462776fca0..f2eadc1cc3da8 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -52,7 +52,7 @@ collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, if (!S || !Var) return; - llvm::SmallPtrSet<const clang::DeclRefExpr *, 16> DREs = + const llvm::SmallPtrSet<const clang::DeclRefExpr *, 16> DREs = clang::tidy::utils::decl_ref_expr::allDeclRefExprs(*Var, *S, Var->getASTContext()); diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp index dc5112e3dccb5..195d1f8f70887 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -36,8 +36,8 @@ void test_same_scope() { } // Variable can be moved to while loop body -// TODO: This is a false positive. Correcting this will require -// loop semantic comprehension and var lifetime analysis. +// FIXME: This is a false positive. Correcting this will require +// loop semantic comprehension and var lifetime analysis. void test_while_loop() { int counter = 0; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'counter' can be declared in a smaller scope @@ -104,10 +104,10 @@ void test_switch_multiple_cases(int value) { // Variable with complex initialization can be moved void test_complex_init() { - int complex = (5 + 3) * 2; - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'complex' can be declared in a smaller scope + int cmplx_expr = (5 + 3) * 2; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'cmplx_expr' can be declared in a smaller scope if (true) { - int doubled = complex * 2; + int doubled = cmplx_expr * 2; } } @@ -192,3 +192,15 @@ void test_lambda() { }; lambda(); } + +// Variable set from function call, used in if clause +// FIXME: This is a false positive because we cannot know if +// func() has side effects or not (since not visible). +int func(); +void test_function_call() { + int i = func(); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'i' can be declared in a smaller scope + if (true) { + i = 0; + } +} >From d678f15461727a47560bea237582ec6126ace2a0 Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Mon, 12 Jan 2026 02:39:25 +0100 Subject: [PATCH 08/10] Update per review comments * Narrow filter, remove code replaced by new filter * Add test case for initializer from function --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 26 ++++--------------- .../checkers/misc/scope-reduction.cpp | 4 +-- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index f2eadc1cc3da8..33800bb32b5b5 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -62,12 +62,12 @@ collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, } void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { - // TODO: Try adding unless(hasParent(declStmt(hasParent(forStmt( to matcher - // to simplify check code. - - // Match on varDecls that are part of a function Finder->addMatcher( - varDecl(hasLocalStorage(), hasAncestor(functionDecl())).bind("var"), + varDecl(hasLocalStorage(), hasAncestor(functionDecl()), + unless(hasParent(declStmt(hasParent(forStmt())))), + unless(hasInitializer(anyOf(callExpr(), cxxMemberCallExpr(), + cxxOperatorCallExpr())))) + .bind("var"), this); } @@ -81,22 +81,6 @@ void ScopeReductionCheck::check( // These variables are already in their optimal scope and shouldn't be // analyzed auto &Parents = Result.Context->getParentMapContext(); - auto ParentNodes = Parents.getParents(DynTypedNode::create(*Var)); - - if (!ParentNodes.empty()) { - if (const auto *Parent = ParentNodes[0].get<Stmt>()) { - if (isa<DeclStmt>(Parent)) { - // Check if DeclStmt's parent is ForStmt - auto GrandParentNodes = Parents.getParents(*Parent); - if (!GrandParentNodes.empty()) { - if (const auto *GrandParent = GrandParentNodes[0].get<Stmt>()) { - if (isa<ForStmt>(GrandParent)) - return; // Skip for-loop declared variables - } - } - } - } - } const auto *Function = dyn_cast<FunctionDecl>(Var->getDeclContext()); assert(Function); diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp index 195d1f8f70887..f49094150eebd 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -194,12 +194,10 @@ void test_lambda() { } // Variable set from function call, used in if clause -// FIXME: This is a false positive because we cannot know if -// func() has side effects or not (since not visible). +// Should NOT warn. Don't know if func() has side effects int func(); void test_function_call() { int i = func(); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'i' can be declared in a smaller scope if (true) { i = 0; } >From ded30920db0d270257d33452cde0b456a9ae5e4b Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Mon, 12 Jan 2026 11:09:59 +0100 Subject: [PATCH 09/10] Update per review comments * Narrow filter by adding parmVarDecl, simplifying check code * Update code comments to match * Add global variable use cases * Narrow filter to exclude global variables in namespaces --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 88 ++++++++++--------- .../checkers/misc/scope-reduction.cpp | 54 ++++++++++++ 2 files changed, 100 insertions(+), 42 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index 33800bb32b5b5..41610c315737a 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -16,8 +16,14 @@ // be simpler. // // The 7-step algorithm used by this checker for scope reduction analysis is: -// 1) Filter out variables declared in for-loop initializations -// - Those variables are already in optimal scope, and can be skipped +// 1) AST Matcher Filtering +// - Only match variables within functions (hasAncestor(functionDecl()) +// - Exclude for-loop declared variables +// (unless(hasParent(declStmt(hasParent(forStmt)))))) +// - Exclude variables with function call initializors +// (unless(hasInitializer(...))) +// - Exclude parameters from analysis +// (unless(parmVarDecl()) // 2) Collect variable uses // - find all DeclRefExpr nodes that reference the variable // 3) Build scope chains @@ -63,7 +69,8 @@ collectVariableUses(const clang::Stmt *S, const clang::VarDecl *Var, void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - varDecl(hasLocalStorage(), hasAncestor(functionDecl()), + varDecl(hasLocalStorage(), unless(hasGlobalStorage()), + hasAncestor(functionDecl()), unless(parmVarDecl()), unless(hasParent(declStmt(hasParent(forStmt())))), unless(hasInitializer(anyOf(callExpr(), cxxMemberCallExpr(), cxxOperatorCallExpr())))) @@ -211,60 +218,57 @@ void ScopeReductionCheck::check( // Step 7: Alternative analysis - check for for-loop initialization // opportunity This only runs if the compound statement analysis didn't find a // smaller scope Only check local variables, not parameters - // TODO: ParmVarDecls maybe excluded for all analysis. - if (!isa<ParmVarDecl>(Var)) { - const ForStmt *CommonForLoop = nullptr; - bool AllUsesInSameForLoop = true; - - for (const auto *Use : Uses) { - const ForStmt *ContainingForLoop = nullptr; - const Stmt *Current = Use; - - // Walk up the AST to find a containing ForStmt - while (Current) { - auto ParentNodes = Parents.getParents(*Current); - if (ParentNodes.empty()) - break; + const ForStmt *CommonForLoop = nullptr; + bool AllUsesInSameForLoop = true; - if (const auto *FS = ParentNodes[0].get<ForStmt>()) { - ContainingForLoop = FS; - break; - } + for (const auto *Use : Uses) { + const ForStmt *ContainingForLoop = nullptr; + const Stmt *Current = Use; - const Stmt *Parent = ParentNodes[0].get<Stmt>(); - if (!Parent) { - // Handle Decl parents like we do in the existing logic - if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { - auto DeclParentNodes = Parents.getParents(*DeclParent); - if (!DeclParentNodes.empty()) - Parent = DeclParentNodes[0].get<Stmt>(); - } - if (!Parent) - break; - } - Current = Parent; - } + // Walk up the AST to find a containing ForStmt + while (Current) { + auto ParentNodes = Parents.getParents(*Current); + if (ParentNodes.empty()) + break; - if (!ContainingForLoop) { - AllUsesInSameForLoop = false; + if (const auto *FS = ParentNodes[0].get<ForStmt>()) { + ContainingForLoop = FS; break; } - if (!CommonForLoop) { - CommonForLoop = ContainingForLoop; - } else if (CommonForLoop != ContainingForLoop) { - AllUsesInSameForLoop = false; - break; + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) { + // Handle Decl parents like we do in the existing logic + if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { + auto DeclParentNodes = Parents.getParents(*DeclParent); + if (!DeclParentNodes.empty()) + Parent = DeclParentNodes[0].get<Stmt>(); + } + if (!Parent) + break; } + Current = Parent; + } + + if (!ContainingForLoop) { + AllUsesInSameForLoop = false; + break; } + if (!CommonForLoop) { + CommonForLoop = ContainingForLoop; + } else if (CommonForLoop != ContainingForLoop) { + AllUsesInSameForLoop = false; + break; + } + } + if (AllUsesInSameForLoop && CommonForLoop) { diag(Var->getLocation(), "variable '%0' can be declared in for-loop initialization") << Var->getName(); return; } - } } } // namespace clang::tidy::misc diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp index f49094150eebd..41b7b65358188 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -169,6 +169,28 @@ void test_unused_variable() { // Global variable - should NOT be processed int global_var = 100; +namespace GlobalTestNamespace { + int namespaced_global = 200; + + // Function using global variables - should NOT warn + void test_global_usage() { + int local = global_var + namespaced_global; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'local' can be declared in a smaller scope + if (true) { + local *= 2; + } + } + + // Global vars used in smaller scopes. Should NOT be detected. + void test_globals_not_detected() { + if (true) { + global_var = 300; + namespaced_global = 400; + int result = global_var + namespaced_global; + } + } +} + // Static local variable - should NOT warn void test_static_variable() { static int static_var = 0; // Should NOT warn - static variables have different semantics @@ -202,3 +224,35 @@ void test_function_call() { i = 0; } } + +// Variable used inside a loop. +// Should NOT warn. +// FIXME: temp needs to persist across loop iterations, therefore cannot move +// Requires more sophisticated analysis. +void test_for_loop_reuse() { + int temp = 0; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'temp' can be declared in a smaller scope + for (int i = 0; i<10; i++) { + temp += i; + } +} + +// Variable can be moved closer to lambda usage +void test_lambda_movable() { + int local = 5; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'local' can be declared in a smaller scope + + if (true) { + auto lambda = [local]() { + return local *3; + }; + } +} + +// Variable declared but never used with empty scope after +void test_unused_empty_scope() { + int unused = 42; // Should NOT warn - this checker only handles scope reduction + if (true) { + // empty scope, variable not used here + } +} >From 7759f03cdadf9fc11463c0dcd3e08ce70f82df1e Mon Sep 17 00:00:00 2001 From: Vince Bridgers <[email protected]> Date: Mon, 12 Jan 2026 12:27:13 +0100 Subject: [PATCH 10/10] update * Manually format code that clang-format missed :/ --- .../clang-tidy/misc/ScopeReductionCheck.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp index 41610c315737a..08f6a844d3be8 100644 --- a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -263,12 +263,12 @@ void ScopeReductionCheck::check( } } - if (AllUsesInSameForLoop && CommonForLoop) { - diag(Var->getLocation(), - "variable '%0' can be declared in for-loop initialization") - << Var->getName(); - return; - } + if (AllUsesInSameForLoop && CommonForLoop) { + diag(Var->getLocation(), + "variable '%0' can be declared in for-loop initialization") + << Var->getName(); + return; + } } } // namespace clang::tidy::misc _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
