This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add misc-for-loop-iterator-invalidation check.
Needs ReviewPublic

Authored by staronj on Apr 19 2017, 6:25 AM.



This is the first version of a check looking for operator or method calls in C++11 for loop on iterated object that may lead to iterator invalidation and therefore to undefined behavior.

In general, it's hard to tell if call may lead to iterator invalidation. Check proposes heuristics and white-list to solve this problem. For heuristics description see ForLoopIteratorInvalidationCheck.cpp:104.

Diff Detail

Event Timeline

staronj created this revision.Apr 19 2017, 6:25 AM

I think this check belongs to Static Analyzer.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: zaks.anna.
Prazek edited edge metadata.Apr 19 2017, 9:56 AM

I think this check belongs to Static Analyzer.

Why do you think so? We don't need path sensitive analysis to do it.
We just check if there is any instruction inside the for loop that looks like changing the container. It seems that the heuristic
is very good and for example it found code that was doing extra lookup in unorderd map

mgehre added a subscriber: mgehre.Apr 19 2017, 11:59 AM

Did you run this check over the llvm/clang code base?


nit: given
and maybe call the function HasEquivalentConstSubstitute?

mgehre added inline comments.Apr 19 2017, 12:06 PM

why should this be OK?


Why is this not triggering the diagnostic?

Prazek added inline comments.Apr 19 2017, 3:15 PM

function names start with lowerCase


This option has to be documented


because we exit the loop after invalidation with break.


lists are not ivalidated and we have seen cases in LLVM where we did loop on the list and add new things to it. This is default option to prevent false positives, but it can be changed.

Prazek added inline comments.Apr 19 2017, 3:25 PM

The docs should also contain the info about

  • the heuristic
  • handling of loop exits
Prazek added inline comments.Apr 20 2017, 2:44 AM

What about pointers type? It is probably uncommon, but the fix should be easy.

staronj updated this revision to Diff 96299.Apr 23 2017, 2:52 AM
staronj marked 4 inline comments as done.

Renames helper functions to start with lowercase.
HaveEqivalent... -> hasEquivalent....
Adds heuristic and option description to documentation.
Adds note about breaking from loop to documentation.


The case with pointer types is not so obvious. Look at following example:
std::vector<int> vec1, vec2;
auto p = &vec1;
for (auto x: *p) {

p = &vec2;
p -> push_back(x); // Completely valid call.


We can't assume that pointer will not change, so we can't assume that pointer is still pointing to iterated container.

Prazek added inline comments.Apr 23 2017, 4:19 AM

ok good point, it would require much more effort

JDevlieghere added a project: Restricted Project.May 11 2017, 10:53 AM
JDevlieghere added a subscriber: JDevlieghere.