[clang-tools-extra] [clang-tidy] Fix modernize-loop-convert by introducing space (PR #202015)
Gaurav Dhingra via cfe-commits
cfe-commits at lists.llvm.org
Sat Jun 6 00:29:21 PDT 2026
https://github.com/gxyd updated https://github.com/llvm/llvm-project/pull/202015
>From 4b091596e9dfc7f2dd050c2b6acea83e9b27e2a1 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Fri, 5 Jun 2026 13:56:47 +0530
Subject: [PATCH 1/3] [clang-tidy] Fix modernize-loop-convert by introducing
space
Introduce leading space for a dereference operator without leading
space, which follows a keyword in loop conversion body
Fixes #105508
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 19 +++
.../Inputs/loop-convert/structures.h | 2 +-
.../checkers/modernize/loop-convert-basic.cpp | 130 ++++++++++++++++++
3 files changed, 150 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 1ba209fed5a27..1962c83db5aa8 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -703,6 +703,25 @@ void LoopConvertCheck::doConversion(
ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow
? VarNameOrStructuredBinding + "."
: VarNameOrStructuredBinding;
+ // In cases like `delete*it`, we can't just change it to `deleteit`,
+ // we need to introduce space after `delete` to make it `delete it`.
+ Token StarToken;
+ if (Usage.Kind == Usage::UK_Default &&
+ !Lexer::getRawToken(Usage.Range.getBegin(), StarToken,
+ Context->getSourceManager(), getLangOpts(),
+ false) &&
+ StarToken.is(tok::star)) {
+ std::optional<Token> PrevToken = Lexer::findPreviousToken(
+ Usage.Range.getBegin(), Context->getSourceManager(),
+ getLangOpts(), true);
+ if (PrevToken) {
+ // Check whether StarToken has leading space or not
+ const bool StarTokenHasNoLeadingSpace =
+ PrevToken->getEndLoc() == StarToken.getLocation();
+ if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
+ ReplaceText = " " + ReplaceText;
+ }
+ }
const DynTypedNodeList Parents = Context->getParents(*Usage.Expression);
if (Parents.size() == 1) {
if (const auto *Paren = Parents[0].get<ParenExpr>()) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
index 839511da30631..64ea452c0062f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
@@ -13,7 +13,7 @@ extern "C" {
extern int printf(const char *restrict, ...);
}
-struct Val {int X; void g(); };
+struct Val {int X; void g(); int** f(); };
struct MutableVal {
void constFun(int) const;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
index 2f744eb8f1c9b..3f9c4d8200ad1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
@@ -1009,3 +1009,133 @@ void test() {
}
}
} // namespace GH109083
+
+namespace GH105508 {
+void test() {
+ Nested<int*> v1;
+ for (auto it=v1.begin(); it != v1.end(); ++it) {
+ delete* it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v1) {
+ // CHECK-FIXES-NEXT: delete it;
+
+ Nested<int*> v2;
+ for (auto it=v2.begin(); it != v2.end(); ++it) {
+ delete* it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v2) {
+ // CHECK-FIXES-NEXT: delete it;
+
+ Nested<int*> v3;
+ for (auto it=v3.begin(); it != v3.end(); ++it) {
+ delete/* random comment */*it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v3) {
+ // CHECK-FIXES-NEXT: delete/* random comment */it;
+
+ Nested<int*> v4;
+ for (auto it=v4.begin(); it != v4.end(); ++it) {
+ sizeof* it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v4) {
+ // CHECK-FIXES-NEXT: sizeof it;
+
+ Nested<int*> v5;
+ for (auto it=v5.begin(); it != v5.end(); ++it) {
+ sizeof* it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v5) {
+ // CHECK-FIXES-NEXT: sizeof it;
+
+ Nested<int*> v6;
+ for (auto it=v6.begin(); it != v6.end(); ++it) {
+ sizeof/* random comment */*it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v6) {
+ // CHECK-FIXES-NEXT: sizeof/* random comment */it;
+
+ Nested<Val> v7;
+ for (auto it=v7.begin(); it != v7.end(); ++it) {
+ delete *it->f();
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v7) {
+ // CHECK-FIXES-NEXT: delete *it.f();
+
+ Nested<Val> v8;
+ for (auto it=v8.begin(); it != v8.end(); ++it) {
+ delete*it->f();
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v8) {
+ // CHECK-FIXES-NEXT: delete*it.f();
+
+ Nested<Val> v9;
+ for (auto it=v9.begin(); it != v9.end(); ++it) {
+ sizeof *it->f();
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v9) {
+ // CHECK-FIXES-NEXT: sizeof *it.f();
+
+ Nested<Val> v10;
+ for (auto it=v10.begin(); it != v10.end(); ++it) {
+ sizeof*it->f();
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v10) {
+ // CHECK-FIXES-NEXT: sizeof*it.f();
+
+ Nested<int*> v11;
+ for (auto it = v11.begin(); it != v11.end(); ++it) {
+ auto H1 = [&it]() { delete*it; };
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v11) {
+ // CHECK-FIXES-NEXT: auto H1 = [&it]() { delete it; };
+
+ Nested<int*> v12;
+ for (auto it = v12.begin(); it != v12.end(); ++it) {
+ auto H1 = [&it]() { return*it; };
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v12) {
+ // CHECK-FIXES-NEXT: auto H1 = [&it]() { return it; };
+}
+
+int* test2() {
+ Nested<int*> v;
+ for (auto it=v.begin(); it != v.end(); ++it) {
+ return*it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v) {
+ // CHECK-FIXES-NEXT: return it;
+}
+
+int* test3() {
+ Nested<int*> v;
+ for (auto it=v.begin(); it != v.end(); ++it) {
+ return* it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v) {
+ // CHECK-FIXES-NEXT: return it;
+}
+
+int* test4() {
+ Nested<int*> v;
+ for (auto it=v.begin(); it != v.end(); ++it) {
+ return *it;
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v) {
+ // CHECK-FIXES-NEXT: return it;
+}
+} // namespace GH105508
>From eed48446eb4a14bace6e206edcff4e9137857ad6 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sat, 6 Jun 2026 12:30:44 +0530
Subject: [PATCH 2/3] add release notes entry
---
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d2db3dc2b3e78..2aec70baf2d71 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -416,6 +416,12 @@ Changes in existing checks
positives on project headers that use the same name as a standard library
header.
+- Improve :doc:`modernize-loop-convert
+ <clang-tidy/checks/modernize/loop-convert>` checks to insert a space when
+ replacing ``*it`` with the loop variable in expressions like ``delete*it``
+ , where the missing space would cause the keyword and the new variable to
+ merge into a single identifier.
+
- Improved :doc:`modernize-pass-by-value
<clang-tidy/checks/modernize/pass-by-value>` check by adding `IgnoreMacros`
option to suppress warnings in macros.
>From c227c12114663610a1c83a0ac106c50edc3e04b5 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sat, 6 Jun 2026 12:58:09 +0530
Subject: [PATCH 3/3] refactor to not use string concatenation
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 1962c83db5aa8..2f342490d76e6 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -698,13 +698,10 @@ void LoopConvertCheck::doConversion(
std::string ReplaceText;
SourceRange Range = Usage.Range;
if (Usage.Expression) {
- // If this is an access to a member through the arrow operator, after
- // the replacement it must be accessed through the '.' operator.
- ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow
- ? VarNameOrStructuredBinding + "."
- : VarNameOrStructuredBinding;
- // In cases like `delete*it`, we can't just change it to `deleteit`,
- // we need to introduce space after `delete` to make it `delete it`.
+ // Decide whether `ReplaceText` needs to be pre-appended with a space
+ // or not. In cases like `delete*it`, we can't just change it to
+ // `deleteit` we need to introduce space after `delete` to make it
+ // `delete it`.
Token StarToken;
if (Usage.Kind == Usage::UK_Default &&
!Lexer::getRawToken(Usage.Range.getBegin(), StarToken,
@@ -719,9 +716,14 @@ void LoopConvertCheck::doConversion(
const bool StarTokenHasNoLeadingSpace =
PrevToken->getEndLoc() == StarToken.getLocation();
if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
- ReplaceText = " " + ReplaceText;
+ ReplaceText = " ";
}
}
+ // If this is an access to a member through the arrow operator, after
+ // the replacement it must be accessed through the '.' operator.
+ ReplaceText += Usage.Kind == Usage::UK_MemberThroughArrow
+ ? VarNameOrStructuredBinding + "."
+ : VarNameOrStructuredBinding;
const DynTypedNodeList Parents = Context->getParents(*Usage.Expression);
if (Parents.size() == 1) {
if (const auto *Paren = Parents[0].get<ParenExpr>()) {
More information about the cfe-commits
mailing list