[clang-tools-extra] Add bugprone-loop-variable-copied-then-modified clang-tidy check. (PR #157213)

[Julia Hansbrough via cfe-commits]

flowerhack wrote:

@nicovank : Thanks for the thoughtful and detailed comments & review.  Replying to your toplevel comment here:

> Suggesting const when the variable is modified in the loop always produces invalid code (this is the case for the highlighted example in the documentation): suggesting const makes little sense when we know/assume the variable is modified.

I performed an experiment where I ran this check over the Chromium codebase.  After filtering out uninteresting code (`*_test.cc` files, etc), it returned about 200 results.  Of those results, I picked about 50 to try handling by applying the naive fix—"if I switch this to a const ref, does it still compile?"—and in 70% of cases, the naive fix compiled without incident.  (I believe that the disparity between "things that the ExprMutationAnalyzer think represents a mutation" vs "things that the `const` designation thinks represents a mutation" is due to the ExprMutationAnalyzer's more conservative semantics.)

The remaining cases, to my eye, seemed like complicated enough situations that they would in fact benefit from more thought/care from the programmer, and it seems fine that the fix-it won't work out-of-the-box for those situations (generally require a judgment call from the programmer to either switch the variable in the loop to a non-const reference, or make the copy explicit in the loop).

So, my hope with the fix-it would be to offer a simple suggestion that would work "most" of the time to minimize disruption to developers.

That does mean some fixes suggested by this will not necessarily be bugfixes but rather a style enforcement—"prefer const references or references over copies in loop variables"— and that does make this something of an opinionated check, to be sure.  And the `IgnoreInexpensiveVariables` toggle does exist for those who'd like some benefit from this check with a higher signal-to-noise ratio.  But I do think it's valuable to have a version of the check that's concerned about *all* variables, since we've observed errors of this type involving inexpensive variables.

> One idea maybe is to check if the copy is then used after modification. If it is not, that's a strong signal the copy was unintended. But this seems tricky.

I think such a modification would fail to catch many of the cases we care about, unfortunately.  A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue.  In this case the field isn't "used" again inside the body of that function.
> One idea maybe is to check if the copy is then used after modification. If it is not, that's a strong signal the copy was unintended. But this seems tricky.

I think such a modification would fail to catch many of the cases we care about, unfortunately.  A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue.  In this case the field isn't "used" again inside the body of that function.


https://github.com/llvm/llvm-project/pull/157213