[clang-tidy] Fix loop-convert for iterators with non-trivial destructors by yashaggarwal85d · Pull Request #236084 · llvm/llvm-project

yashaggarwal85d · 2026-10-27T11:48:30Z

When the iterator type has a non-trivial destructor, the end() call in the loop condition is wrapped in a CXXBindTemporaryExpr. The matcher for the loop bound only used ignoringParenImpCasts, which doesn't skip it, so loops like for (It it = c.begin(); it != c.end(); ++it) were never flagged. This also hits MSVC STL iterators in debug builds (_ITERATOR_DEBUG_LEVEL=2).

I added ignoringImplicit inside it so the bound temporary is skipped too. The old materializeTemporaryExpr(...) alternative is dropped because ignoringParenImpCasts already looks through MaterializeTemporaryExpr, so it didn't match anything the first branch didn't.

Fixes #234990

This was AI-assisted; I reviewed and tested the change myself.

github-actions · 2026-10-27T11:48:48Z

Hello @yashaggarwal85d 👋

Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.

All contributions to LLVM must follow our LLVM AI Tool Use Policy. In particular, if you used AI while working on this PR, remember to add a note to the PR description.
The LLVM Code-Review Policy and Practices document contains practical information about the PR process, including how patches are reviewed and accepted, and who can review a PR.
Our LLVM Developer Policy describes our expectations for code quality, commit summaries and contains notes on our CI system.

Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description.

Frequently asked questions

How do I add reviewers?

This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically.

You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using @ followed by their GitHub username.

What if there are no comments?

If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers.

Are any special GitHub settings required to contribute to LLVM?

We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details.

If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse.

Thank you,
The LLVM Community

llvmorg-github-actions · 2026-10-27T11:49:24Z

@llvm/pr-subscribers-clang-tools-extra

Author: Yash Aggarwal (yashaggarwal85d)

Changes

When the iterator type has a non-trivial destructor, the end() call in the loop condition is wrapped in a CXXBindTemporaryExpr. The matcher for the loop bound only used ignoringParenImpCasts, which doesn't skip it, so loops like for (It it = c.begin(); it != c.end(); ++it) were never flagged. This also hits MSVC STL iterators in debug builds (_ITERATOR_DEBUG_LEVEL=2).

I added ignoringImplicit inside it so the bound temporary is skipped too. The old materializeTemporaryExpr(...) alternative is dropped because ignoringParenImpCasts already looks through MaterializeTemporaryExpr, so it didn't match anything the first branch didn't.

Fixes #234990

This was AI-assisted; I reviewed and tested the change myself.

Full diff: https://github.com/llvm/llvm-project/pull/236084.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp (+1-2)
(modified) clang-tools-extra/docs/ReleaseNotes.md (+5)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp (+43)

diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 75bf2a73259000..d5b252b66522c3 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -197,8 +197,7 @@ static StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
   const StatementMatcher IteratorBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
                      declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
-                 ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
-                 materializeTemporaryExpr(ignoringParenImpCasts(
+                 ignoringParenImpCasts(ignoringImplicit(
                      expr(EndCallMatcher).bind(EndCallName)))));
 
   const StatementMatcher IteratorComparisonMatcher = expr(ignoringParenImpCasts(
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 310b084c04eb04..45c5f30d05be7f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -283,6 +283,11 @@ infrastructure are described first, followed by tool-specific sections.
   nested expressions involving different macros or a mix of macro and
   non-macro operands.
 
+- Improved {doc}`modernize-loop-convert
+  <clang-tidy/checks/modernize/loop-convert>` check to also convert iterator
+  loops that call `end()` in the condition when the iterator type has a
+  non-trivial destructor.
+
 - Fixed a crash in {doc}`modernize-raw-string-literal
   <clang-tidy/checks/modernize/raw-string-literal>` on synthetic string
   literals created for raw user-defined literal operators, such as `12_w`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp
new file mode 100644
index 00000000000000..c19f21a59183c0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s modernize-loop-convert %t
+
+struct Container {
+  struct iterator {
+    ~iterator() {}
+    int &operator*() const;
+    iterator &operator++();
+    bool operator!=(const iterator &) const;
+  };
+  iterator begin();
+  iterator end();
+};
+
+void f(Container &C) {
+  for (Container::iterator It = C.begin(); It != C.end(); ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(); It != (C.end()); ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(); C.end() != It; ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(), E = C.end(); It != E; ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+}
+
+void negative(Container &C, Container &D) {
+  for (Container::iterator It = C.begin(); It != D.end(); ++It)
+    (void)*It;
+}

llvmorg-github-actions · 2026-10-27T11:49:25Z

@llvm/pr-subscribers-clang-tidy

Author: Yash Aggarwal (yashaggarwal85d)

Changes

When the iterator type has a non-trivial destructor, the end() call in the loop condition is wrapped in a CXXBindTemporaryExpr. The matcher for the loop bound only used ignoringParenImpCasts, which doesn't skip it, so loops like for (It it = c.begin(); it != c.end(); ++it) were never flagged. This also hits MSVC STL iterators in debug builds (_ITERATOR_DEBUG_LEVEL=2).

I added ignoringImplicit inside it so the bound temporary is skipped too. The old materializeTemporaryExpr(...) alternative is dropped because ignoringParenImpCasts already looks through MaterializeTemporaryExpr, so it didn't match anything the first branch didn't.

Fixes #234990

This was AI-assisted; I reviewed and tested the change myself.

Full diff: https://github.com/llvm/llvm-project/pull/236084.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp (+1-2)
(modified) clang-tools-extra/docs/ReleaseNotes.md (+5)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp (+43)

diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 75bf2a73259000..d5b252b66522c3 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -197,8 +197,7 @@ static StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
   const StatementMatcher IteratorBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
                      declRefExpr(to(varDecl(equalsBoundNode(EndVarName))))),
-                 ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
-                 materializeTemporaryExpr(ignoringParenImpCasts(
+                 ignoringParenImpCasts(ignoringImplicit(
                      expr(EndCallMatcher).bind(EndCallName)))));
 
   const StatementMatcher IteratorComparisonMatcher = expr(ignoringParenImpCasts(
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 310b084c04eb04..45c5f30d05be7f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -283,6 +283,11 @@ infrastructure are described first, followed by tool-specific sections.
   nested expressions involving different macros or a mix of macro and
   non-macro operands.
 
+- Improved {doc}`modernize-loop-convert
+  <clang-tidy/checks/modernize/loop-convert>` check to also convert iterator
+  loops that call `end()` in the condition when the iterator type has a
+  non-trivial destructor.
+
 - Fixed a crash in {doc}`modernize-raw-string-literal
   <clang-tidy/checks/modernize/raw-string-literal>` on synthetic string
   literals created for raw user-defined literal operators, such as `12_w`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp
new file mode 100644
index 00000000000000..c19f21a59183c0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-nontrivial-iterator-dtor.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s modernize-loop-convert %t
+
+struct Container {
+  struct iterator {
+    ~iterator() {}
+    int &operator*() const;
+    iterator &operator++();
+    bool operator!=(const iterator &) const;
+  };
+  iterator begin();
+  iterator end();
+};
+
+void f(Container &C) {
+  for (Container::iterator It = C.begin(); It != C.end(); ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(); It != (C.end()); ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(); C.end() != It; ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+
+  for (Container::iterator It = C.begin(), E = C.end(); It != E; ++It)
+    (void)*It;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : C)
+  // CHECK-FIXES-NEXT: (void)It;
+}
+
+void negative(Container &C, Container &D) {
+  for (Container::iterator It = C.begin(); It != D.end(); ++It)
+    (void)*It;
+}

yashaggarwal85d · 2026-10-31T06:37:09Z

Read the policies, thanks

github-actions · 2026-10-31T08:19:34Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

zeyi2

Mostly LG! One comment for the tests.

Also please make your email address public :)

zeyi2 · 2026-10-31T08:31:20Z

@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s modernize-loop-convert %t


Could we move the test to existing loop-convert-extra.cpp?

There are also iterator and container types in Inputs/loop-convert/structures.h that we may be able to reuse or extend, rather than introducing a new Container here.

Thanks! Moved the tests into loop-convert-extra.cpp and dropped the separate file.

…a.cpp

yashaggarwal85d · 2026-11-02T05:17:52Z

Mostly LG! One comment for the tests.

Also please make your email address public :)

Thanks @zeyi2 for the review. I have adressed them and made my email public as well. Thanks!

zeyi2

LGTM

A test run on llvm/llvm-project#236084 marked all 884 warnings TP with 873 "Same as #1" rationales and no reproducers, missing 4 real FPs. Require a source read per row, name the shared pattern in "Same as", and choose reproducer rows by pattern rather than by expected verdict. Amp-Thread-ID: https://ampcode.com/threads/T-01a19acc-7034-72fe-908f-15ca2649429a Co-authored-by: Zeyi Xu <[email protected]>

A run reviewing all 884 warnings of llvm/llvm-project#236084 hit the 45-minute step limit, which also skipped the second pass and the final report check. Raise the step limit to 120 minutes with per-pass timeouts so the check always runs, and have the agent save verdicts every 20 rows so a timeout keeps the finished rows. Amp-Thread-ID: https://ampcode.com/threads/T-01a19acc-7034-72fe-908f-15ca2649429a Co-authored-by: Zeyi Xu <[email protected]>

[clang-tidy] Fix loop-convert for iterators with non-trivial destructors

81b3b4e

yashaggarwal85d requested review from vbvictor and zeyi2 as code owners October 27, 2026 11:48

llvmorg-github-actions Bot added clang-tools-extra clang-tidy labels Oct 27, 2026

zeyi2 reviewed Oct 31, 2026

View reviewed changes

zeyi2 mentioned this pull request Oct 31, 2026

[Test] modernize-loop-convert clang-tidy-infra/CTIT#287

Open

yashaggarwal85d added 2 commits November 2, 2026 04:14

Move the non-trivial iterator destructor tests into loop-convert-extr…

7f067bd

…a.cpp

Merge branch 'main' into fix-loop-convert-nontrivial-iterator-dtor

e55f6f4

yashaggarwal85d requested a review from zeyi2 November 2, 2026 05:18

zeyi2 approved these changes Nov 2, 2026

View reviewed changes

Navigation Menu

Repository navigation

[clang-tidy] Fix loop-convert for iterators with non-trivial destructors - #236084

[clang-tidy] Fix loop-convert for iterators with non-trivial destructors#236084
yashaggarwal85d wants to merge 3 commits into
llvm:mainfrom
yashaggarwal85d:fix-loop-convert-nontrivial-iterator-dtor

yashaggarwal85d commented Oct 27, 2026

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

llvmorg-github-actions Bot commented Oct 27, 2026

Uh oh!

llvmorg-github-actions Bot commented Oct 27, 2026

Uh oh!

yashaggarwal85d commented Oct 31, 2026

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

zeyi2 left a comment

Uh oh!

zeyi2 Oct 31, 2026

Uh oh!

yashaggarwal85d Nov 2, 2026

Uh oh!

yashaggarwal85d commented Nov 2, 2026

Uh oh!

zeyi2 left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,43 @@
		// RUN: %check_clang_tidy %s modernize-loop-convert %t

Conversation

yashaggarwal85d commented Oct 27, 2026

Uh oh!

github-actions Bot commented Oct 27, 2026

Frequently asked questions

Uh oh!

llvmorg-github-actions Bot commented Oct 27, 2026

Uh oh!

llvmorg-github-actions Bot commented Oct 27, 2026

Uh oh!

yashaggarwal85d commented Oct 31, 2026

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

zeyi2 left a comment

Choose a reason for hiding this comment

Uh oh!

zeyi2 Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

yashaggarwal85d Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

yashaggarwal85d commented Nov 2, 2026

Uh oh!

zeyi2 left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants