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 <[email protected]> Date: Fri, 5 Jun 2026 13:56:47 +0530 Subject: [PATCH 01/12] [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 <[email protected]> Date: Sat, 6 Jun 2026 12:30:44 +0530 Subject: [PATCH 02/12] 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 <[email protected]> Date: Sat, 6 Jun 2026 12:58:09 +0530 Subject: [PATCH 03/12] 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 <[email protected]> Date: Sat, 6 Jun 2026 13:04:58 +0530 Subject: [PATCH 04/12] 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 <[email protected]> Date: Sun, 7 Jun 2026 16:19:25 +0530 Subject: [PATCH 05/12] 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 <[email protected]> Date: Sun, 7 Jun 2026 16:37:39 +0530 Subject: [PATCH 06/12] 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 <[email protected]> Date: Sun, 7 Jun 2026 16:41:07 +0530 Subject: [PATCH 07/12] 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 <[email protected]> Date: Sun, 7 Jun 2026 18:00:42 +0530 Subject: [PATCH 08/12] 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 <[email protected]> Date: Sun, 7 Jun 2026 18:29:14 +0530 Subject: [PATCH 09/12] 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 <[email protected]> Date: Wed, 10 Jun 2026 18:01:00 +0530 Subject: [PATCH 10/12] 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 <[email protected]> Date: Wed, 10 Jun 2026 18:09:20 +0530 Subject: [PATCH 11/12] 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 <[email protected]> Date: Wed, 10 Jun 2026 18:17:12 +0530 Subject: [PATCH 12/12] 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; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
