[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
Wed Jun 10 06:15:45 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 01/13] [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 02/13] 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 03/13] 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>()) {
>From e34f3089c589eae84e1bf8d65810a423d9d919ab Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sat, 6 Jun 2026 13:04:58 +0530
Subject: [PATCH 04/13] fix formatting issue
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 2f342490d76e6..875654b81fbd1 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -722,8 +722,8 @@ void LoopConvertCheck::doConversion(
// 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;
+ ? VarNameOrStructuredBinding + "."
+ : VarNameOrStructuredBinding;
const DynTypedNodeList Parents = Context->getParents(*Usage.Expression);
if (Parents.size() == 1) {
if (const auto *Paren = Parents[0].get<ParenExpr>()) {
>From 7b15c997adbb1047557ac2ca147502ea0c6201c0 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sun, 7 Jun 2026 16:19:25 +0530
Subject: [PATCH 05/13] refactor code to a separate function and update release
notes entry
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 45 ++++++++++---------
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
2 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 875654b81fbd1..efa7be11d2585 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -624,6 +624,28 @@ void LoopConvertCheck::getAliasRange(SourceManager &SM, SourceRange &Range) {
SourceRange(Range.getBegin(), Range.getEnd().getLocWithOffset(Offset));
}
+// Returns `true` if `ReplaceText` needs to be prepended with a space else
+// false. 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`.
+static bool requiresLeadingSpace(SourceManager &SourceMgr,
+ const LangOptions &LangOpts,
+ SourceRange Range) {
+ Token StarToken;
+ if (!Lexer::getRawToken(Range.getBegin(), StarToken, SourceMgr, LangOpts,
+ false) &&
+ StarToken.is(tok::star)) {
+ std::optional<Token> PrevToken =
+ Lexer::findPreviousToken(Range.getBegin(), SourceMgr, LangOpts, true);
+ assert(PrevToken && "Expected a token before the dereference operator");
+ // Check whether StarToken has leading space or not
+ const bool StarTokenHasNoLeadingSpace =
+ PrevToken->getEndLoc() == StarToken.getLocation();
+ if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
+ return true;
+ }
+ return false;
+}
+
/// Computes the changes needed to convert a given for loop, and
/// applies them.
void LoopConvertCheck::doConversion(
@@ -698,27 +720,10 @@ void LoopConvertCheck::doConversion(
std::string ReplaceText;
SourceRange Range = Usage.Range;
if (Usage.Expression) {
- // 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,
- 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 = " ";
- }
- }
+ requiresLeadingSpace(Context->getSourceManager(), getLangOpts(),
+ Usage.Range))
+ 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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2aec70baf2d71..52431c90e8850 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -418,8 +418,8 @@ Changes in existing checks
- 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
+ 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
>From 8f4cbb6f67a7473f61c25913665ecb8d6f3a587f Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sun, 7 Jun 2026 16:37:39 +0530
Subject: [PATCH 06/13] refactor `requiresLeadingSpace`
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 47 ++++++++++---------
1 file changed, 24 insertions(+), 23 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index efa7be11d2585..eaf50ee534a5f 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -552,6 +552,29 @@ static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) {
return false;
}
+// Returns true if the replacement text needs a leading space to avoid merging
+// with the preceding token. This occurs when `*it` is immediately adjacent to
+// a keyword, e.g. `delete*it`, where replacing `*it` with `it` would
+// incorrectly produce `deleteit`. So we insert a space b/w `delete` and `it`.
+static bool requiresLeadingSpace(SourceManager &SourceMgr,
+ const LangOptions &LangOpts,
+ SourceLocation BeginLocation) {
+ Token StarToken;
+ if (!Lexer::getRawToken(BeginLocation, StarToken, SourceMgr, LangOpts,
+ false) &&
+ StarToken.is(tok::star)) {
+ std::optional<Token> PrevToken =
+ Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
+ assert(PrevToken && "Expected a token before the dereference operator");
+ // Check whether StarToken has leading space or not
+ const bool StarTokenHasNoLeadingSpace =
+ PrevToken->getEndLoc() == BeginLocation;
+ if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
+ return true;
+ }
+ return false;
+}
+
LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
MaxCopySize(Options.get("MaxCopySize", 16ULL)),
@@ -624,28 +647,6 @@ void LoopConvertCheck::getAliasRange(SourceManager &SM, SourceRange &Range) {
SourceRange(Range.getBegin(), Range.getEnd().getLocWithOffset(Offset));
}
-// Returns `true` if `ReplaceText` needs to be prepended with a space else
-// false. 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`.
-static bool requiresLeadingSpace(SourceManager &SourceMgr,
- const LangOptions &LangOpts,
- SourceRange Range) {
- Token StarToken;
- if (!Lexer::getRawToken(Range.getBegin(), StarToken, SourceMgr, LangOpts,
- false) &&
- StarToken.is(tok::star)) {
- std::optional<Token> PrevToken =
- Lexer::findPreviousToken(Range.getBegin(), SourceMgr, LangOpts, true);
- assert(PrevToken && "Expected a token before the dereference operator");
- // Check whether StarToken has leading space or not
- const bool StarTokenHasNoLeadingSpace =
- PrevToken->getEndLoc() == StarToken.getLocation();
- if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
- return true;
- }
- return false;
-}
-
/// Computes the changes needed to convert a given for loop, and
/// applies them.
void LoopConvertCheck::doConversion(
@@ -722,7 +723,7 @@ void LoopConvertCheck::doConversion(
if (Usage.Expression) {
if (Usage.Kind == Usage::UK_Default &&
requiresLeadingSpace(Context->getSourceManager(), getLangOpts(),
- Usage.Range))
+ Usage.Range.getBegin()))
ReplaceText = " ";
// If this is an access to a member through the arrow operator, after
// the replacement it must be accessed through the '.' operator.
>From 7252c9f68d287e451ef78780cbfc6daf8d90b2c5 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sun, 7 Jun 2026 16:41:07 +0530
Subject: [PATCH 07/13] add a working test case
---
.../clang-tidy/checkers/modernize/loop-convert-basic.cpp | 8 ++++++++
1 file changed, 8 insertions(+)
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 3f9c4d8200ad1..9876ecd9c4477 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
@@ -1107,6 +1107,14 @@ void test() {
// 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; };
+
+ Nested<int*> v13;
+ for (auto it=v13.begin(); it != v13.end(); ++it) {
+ sizeof(*it);
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v13) {
+ // CHECK-FIXES-NEXT: sizeof(it);
}
int* test2() {
>From 05df1e16eebd355b471b7d3095bc6154d85e1434 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sun, 7 Jun 2026 18:00:42 +0530
Subject: [PATCH 08/13] refactor code to have two separate functions
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 30 ++++++---
.../checkers/modernize/loop-convert-basic.cpp | 64 +++++++++++++++++++
2 files changed, 85 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index eaf50ee534a5f..93dc1e8135b12 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -552,6 +552,21 @@ static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) {
return false;
}
+// Returns true if the token at `BeginLocation` is immediately preceded by an
+// identifier or keyword token with no space between them.
+static bool
+isPrecededByAdjacentIdentifierOrKeyword(SourceManager &SourceMgr,
+ const LangOptions &LangOpts,
+ SourceLocation BeginLocation) {
+ std::optional<Token> PrevToken =
+ Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
+ assert(PrevToken && "Expected a token before the dereference operator");
+ // Check whether the token at `BeginLocation` is immediately adjacent to
+ // the previous token with no space between them.
+ const bool IsAdjacentToPrevToken = PrevToken->getEndLoc() == BeginLocation;
+ return PrevToken->isAnyIdentifier() && IsAdjacentToPrevToken;
+}
+
// Returns true if the replacement text needs a leading space to avoid merging
// with the preceding token. This occurs when `*it` is immediately adjacent to
// a keyword, e.g. `delete*it`, where replacing `*it` with `it` would
@@ -563,14 +578,8 @@ static bool requiresLeadingSpace(SourceManager &SourceMgr,
if (!Lexer::getRawToken(BeginLocation, StarToken, SourceMgr, LangOpts,
false) &&
StarToken.is(tok::star)) {
- std::optional<Token> PrevToken =
- Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
- assert(PrevToken && "Expected a token before the dereference operator");
- // Check whether StarToken has leading space or not
- const bool StarTokenHasNoLeadingSpace =
- PrevToken->getEndLoc() == BeginLocation;
- if (PrevToken->isAnyIdentifier() && StarTokenHasNoLeadingSpace)
- return true;
+ return isPrecededByAdjacentIdentifierOrKeyword(SourceMgr, LangOpts,
+ BeginLocation);
}
return false;
}
@@ -738,7 +747,10 @@ void LoopConvertCheck::doConversion(
// removed except in case of a `sizeof` operator call.
const DynTypedNodeList GrandParents = Context->getParents(*Paren);
if (GrandParents.size() != 1 ||
- GrandParents[0].get<UnaryExprOrTypeTraitExpr>() == nullptr) {
+ (GrandParents[0].get<UnaryExprOrTypeTraitExpr>() == nullptr &&
+ !isPrecededByAdjacentIdentifierOrKeyword(
+ Context->getSourceManager(), getLangOpts(),
+ Parents[0].getSourceRange().getBegin()))) {
Range = Paren->getSourceRange();
}
} else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
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 9876ecd9c4477..f5d64a29a53b3 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
@@ -1115,6 +1115,30 @@ void test() {
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
// CHECK-FIXES: for (auto & it : v13) {
// CHECK-FIXES-NEXT: sizeof(it);
+
+ Nested<int*> v14;
+ for (auto it=v14.begin(); it != v14.end(); ++it) {
+ sizeof((*it));
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v14) {
+ // CHECK-FIXES-NEXT: sizeof(it);
+
+ Nested<int*> v15;
+ for (auto it=v15.begin(); it != v15.end(); ++it) {
+ delete(*it);
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v15) {
+ // CHECK-FIXES-NEXT: delete(it);
+
+ Nested<int*> v16;
+ for (auto it=v16.begin(); it != v16.end(); ++it) {
+ delete((*it));
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+ // CHECK-FIXES: for (auto & it : v16) {
+ // CHECK-FIXES-NEXT: delete(it);
}
int* test2() {
@@ -1146,4 +1170,44 @@ int* test4() {
// CHECK-FIXES: for (auto & it : v) {
// CHECK-FIXES-NEXT: return it;
}
+
+int* test5() {
+ 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* test6() {
+ 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* test7() {
+ Nested<int*> v;
+ for (auto it=v.begin(); it != v.end(); ++it) {
+ return((*it) + 1);
+ }
+ // 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 + 1);
+}
+
+int* test8() {
+ Nested<int*> v;
+ for (auto it=v.begin(); it != v.end(); ++it) {
+ return(1 + (*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(1 + it);
+}
} // namespace GH105508
>From 3e901772b753d99424832c1b92d05e292c190e4a Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Sun, 7 Jun 2026 18:29:14 +0530
Subject: [PATCH 09/13] add a check before using an optional value
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 93dc1e8135b12..f85e1fec5677e 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -560,7 +560,8 @@ isPrecededByAdjacentIdentifierOrKeyword(SourceManager &SourceMgr,
SourceLocation BeginLocation) {
std::optional<Token> PrevToken =
Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
- assert(PrevToken && "Expected a token before the dereference operator");
+ if (!PrevToken)
+ return false;
// Check whether the token at `BeginLocation` is immediately adjacent to
// the previous token with no space between them.
const bool IsAdjacentToPrevToken = PrevToken->getEndLoc() == BeginLocation;
>From 88e0a8a82743ea2516f1202a018a95b4a6e4bb43 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Wed, 10 Jun 2026 18:01:00 +0530
Subject: [PATCH 10/13] use `const` qualifier in added functions
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index f85e1fec5677e..c21a0d54159ec 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -555,7 +555,7 @@ static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) {
// Returns true if the token at `BeginLocation` is immediately preceded by an
// identifier or keyword token with no space between them.
static bool
-isPrecededByAdjacentIdentifierOrKeyword(SourceManager &SourceMgr,
+isPrecededByAdjacentIdentifierOrKeyword(const SourceManager &SourceMgr,
const LangOptions &LangOpts,
SourceLocation BeginLocation) {
std::optional<Token> PrevToken =
@@ -572,7 +572,7 @@ isPrecededByAdjacentIdentifierOrKeyword(SourceManager &SourceMgr,
// with the preceding token. This occurs when `*it` is immediately adjacent to
// a keyword, e.g. `delete*it`, where replacing `*it` with `it` would
// incorrectly produce `deleteit`. So we insert a space b/w `delete` and `it`.
-static bool requiresLeadingSpace(SourceManager &SourceMgr,
+static bool requiresLeadingSpace(const SourceManager &SourceMgr,
const LangOptions &LangOpts,
SourceLocation BeginLocation) {
Token StarToken;
>From fd8834ffc13d99a03d36d1a62972b425f7e7ca3c Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Wed, 10 Jun 2026 18:09:20 +0530
Subject: [PATCH 11/13] use `assert` as `assert(PrevToken && "Some Text")`
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index c21a0d54159ec..dc9b6e4d8399a 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -560,8 +560,7 @@ isPrecededByAdjacentIdentifierOrKeyword(const SourceManager &SourceMgr,
SourceLocation BeginLocation) {
std::optional<Token> PrevToken =
Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
- if (!PrevToken)
- return false;
+ assert(PrevToken && "Expected a token before the dereference operator");
// Check whether the token at `BeginLocation` is immediately adjacent to
// the previous token with no space between them.
const bool IsAdjacentToPrevToken = PrevToken->getEndLoc() == BeginLocation;
>From 539f9f7843141db305242c2d07e27d21b51240c3 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Wed, 10 Jun 2026 18:17:12 +0530
Subject: [PATCH 12/13] just use `assert(PrevToken)`
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index dc9b6e4d8399a..4d13d67c153e9 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -560,7 +560,7 @@ isPrecededByAdjacentIdentifierOrKeyword(const SourceManager &SourceMgr,
SourceLocation BeginLocation) {
std::optional<Token> PrevToken =
Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
- assert(PrevToken && "Expected a token before the dereference operator");
+ assert(PrevToken);
// Check whether the token at `BeginLocation` is immediately adjacent to
// the previous token with no space between them.
const bool IsAdjacentToPrevToken = PrevToken->getEndLoc() == BeginLocation;
>From 9b7d0c8f871b1fbc7d106ed4e2c7707f67628934 Mon Sep 17 00:00:00 2001
From: Gaurav Dhingra <gauravdhingra.gxyd at gmail.com>
Date: Wed, 10 Jun 2026 18:39:35 +0530
Subject: [PATCH 13/13] revert to `if (!PrevToken)` for now
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 4d13d67c153e9..c21a0d54159ec 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -560,7 +560,8 @@ isPrecededByAdjacentIdentifierOrKeyword(const SourceManager &SourceMgr,
SourceLocation BeginLocation) {
std::optional<Token> PrevToken =
Lexer::findPreviousToken(BeginLocation, SourceMgr, LangOpts, true);
- assert(PrevToken);
+ if (!PrevToken)
+ return false;
// Check whether the token at `BeginLocation` is immediately adjacent to
// the previous token with no space between them.
const bool IsAdjacentToPrevToken = PrevToken->getEndLoc() == BeginLocation;
More information about the cfe-commits
mailing list