[clang-tidy] modernize-loop-convert: impl const cast iter

[torbjoernk]

Summary

modernize-loop-convert was not detecting implicit casts to
const_iterator as convertible to range-based loops:

std::vector<int> vec{1,2,3,4}
for(std::vector<int>::const_iterator i = vec.begin();
    i != vec.end();
    ++i) { }

Thanks to Don Hinton for advice.

As well, this change adds a note for this check's applicability to code
targeting OpenMP prior version 5 as this check will continue breaking
compilation with -fopenmp. Thanks to Roman Lebedev for pointing this
out.

Fixes PR#35082

[hintonda]

clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp

273 ↗

(On Diff #199153)

I think you need to fix the comment and add checks to these also, which is what the if else was dealing with:

434    // V.begin() returns a user-defined type 'iterator' which, since it's
435    // different from const_iterator, disqualifies these loops from
436    // transformation.
437    dependent<int> V;
438    for (dependent<int>::const_iterator It = V.begin(), E = V.end();
439         It != E; ++It) {
440      printf("Fibonacci number is %d\n", *It);
441    }
442
443    for (dependent<int>::const_iterator It(V.begin()), E = V.end();
444         It != E; ++It) {
445      printf("Fibonacci number is %d\n", *It);
446    }
447  }

[lebedev.ri]

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

[hintonda]

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

[lebedev.ri]

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

[torbjoernk]

clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp

273 ↗

(On Diff #199153)

This seems weird and I reckon my setup is broken as also a bunch of other lit tests (make check-clang-tools) fail.

modernize-loop-convert-basic fails once I change this to the following:

dependent<int> V;
for (dependent<int>::const_iterator It = V.begin(), E = V.end();
     It != E; ++It) {
  printf("Fibonacci number is %d\n", *It);
}
// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (int It : V)
// CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);

for (dependent<int>::const_iterator It(V.begin()), E = V.end();
     It != E; ++It) {
  printf("Fibonacci number is %d\n", *It);
}
// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (int It : V)
// CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);

However, copy&pasted outsite of llvm-lit in a minimal example, it is detected and fixed as expected.

Is it possible that llvm-lit picks up my (older) globally available clang-tidy?

[torbjoernk]

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

[lebedev.ri]

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

Yes and no.
This will prevent the false-positive, but only if the OpenMP is enabled (-fopenmp).
If OpenMP is not enabled, then that won't work, because there won't be anything about OpenMP in AST.
I semi-strongly believe it will be less confusing to only document this false-positive (in check's docs),
instead of trying to prevent it, and reliably failing in half of the cases.

[hintonda]

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable, as far as I'm concerned, it doesn't exist. Most tests of this sort are mocked up to emulate the problem so you can verify it exists and the fix actually addresses it.

[lebedev.ri]

In D61827#1499306, @hintonda wrote:

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

In D61827#1499306, @hintonda wrote:

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?

as far as I'm concerned, it doesn't exist. Most tests of this sort are mocked up to emulate the problem so you can verify it exists and the fix actually addresses it.

[hintonda]

In D61827#1499309, @lebedev.ri wrote:

In D61827#1499306, @hintonda wrote:

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

[lebedev.ri]

In D61827#1499333, @hintonda wrote:

In D61827#1499309, @lebedev.ri wrote:

In D61827#1499306, @hintonda wrote:

In D61827#1499303, @torbjoernk wrote:

In D61827#1499184, @lebedev.ri wrote:

In D61827#1499183, @hintonda wrote:

In D61827#1499160, @lebedev.ri wrote:

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either)

[hintonda]

In D61827#1499335, @lebedev.ri wrote:

In D61827#1499333, @hintonda wrote:

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either)

I'm really not sure I understand what you are trying to say. Are you saying this patch is a regression?

By "false positive", do you mean that diagnoses something it shouldn't change and creates an erroneous fixit? Of that it ignores something it should fix?

[lebedev.ri]

In D61827#1499347, @hintonda wrote:

In D61827#1499335, @lebedev.ri wrote:

In D61827#1499333, @hintonda wrote:

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either)

I'm really not sure I understand what you are trying to say.

Are you saying this patch is a regression?

Not in general, no. This is most certainly an improvement for normal C++ code.

By "false positive", do you mean that diagnoses something it shouldn't change and creates an erroneous fixit?

For normal C++ - not that i know of, can't comment.

For loops that are OpenMP loops - yes, this will be a false-positive with erroneous, compilation-breaking, fixit.
(To be noted, the existing fix-its in this check already are that way.)

But as i said, this is only "for your information", only to be added to docs (if it isn't there already).
As i have said, this can not be reliably avoided, and i don't believe a partial avoidance will be good.

Of that it ignores something it should fix?

That is called false-negative. No, this isn't the case here.

[hintonda]

I pulled down you patch, compiled and ran it. Once I fixed the two problems I mentioned, it ran clean, e.g.:

$ bin/llvm-lit -vv ../../llvm-project/clang-tools-extra/test/clang-tidy/modernize-loop-convert-[be]*.cpp
-- Testing: 2 tests, 2 threads --
PASS: Clang Tools :: clang-tidy/modernize-loop-convert-basic.cpp (1 of 2)
PASS: Clang Tools :: clang-tidy/modernize-loop-convert-extra.cpp (2 of 2)
Testing Time: 0.93s
  Expected Passes    : 2

Also, I don't think the omp issue is a regression, so I wouldn't worry about trying to fix it here.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

131 ↗

(On Diff #199153)

This change is incorrect. The matcher actually needs all three cases. This explains the strange test failures you were seeing.

clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp

274 ↗

(On Diff #199153)

Instead of adding a new test case, please fix the two I mentioned earlier, and 5 more in modernize-loop-convert-extra.cpp. They just need you to add the appropriate CHECK line.

[hintonda]

In D61827#1499368, @lebedev.ri wrote:

In D61827#1499347, @hintonda wrote:

Are you saying this patch is a regression?

Not in general, no. This is most certainly an improvement for normal C++ code.

Good, then this patch can go forward.

I agree that a note that this shouldn't be run on openmp 4. Perhaps a separate patch could check for that and not do anything when compiling an openmp file.

[torbjoernk]

Fixed the issues pointed out by Don Hinton and added note on OpenMP to the check's docs as suggested by Roman Lebedev.

Also fixed a small typo in a comment within the tests.

Side note: Somehow, the various failing tests I had previously where due to the fact that make check-clang-tools was picking my system-installed clang-tidy-8 and not the one I was building... o.O

[lebedev.ri]

Sounds good, thank you!

clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst

262 ↗

(On Diff #199326)

I would add a cross-reference to NOLINT documentation,
and a mention that the check intentionally makes no attempt
to prevent issuing these diagnostics for OpenMP loops.

[hintonda]

Awesome, thanks...

LGTM, but you may want to give @alexfh or @aaron.ballman a day to comment before you commit.

[torbjoernk]

Addressed Roman Lebedev's comment regarding reference to NOLINT in the docs.

As I don't have commit rights, I'd be grateful someone else could commit this patch once it's fully accepted.

[hintonda]

I'll be happy to commit for you, but will give it till tomorrow just to make sure no one else has any additional comments.

Thanks again!

clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst

262 ↗

(On Diff #199483)

Could you reword this last line to remove the double negative? Perhaps something like:

This check makes no attempt to exclude incorrect diagnostics for OpenMP loops prior to OpenMP 5.

[torbjoernk]

minor rewording

[lebedev.ri]

By the way, much to my surprise, this didn't start diagnosing the loop i expected it to start diagnosing:
https://godbolt.org/z/lsJTSS
This is expected?

clang-tidy/modernize/LoopConvertCheck.cpp

if (FixerKind == LFK_Iterator) {
     if (CanonicalBeginType->isPointerType() &&
         CanonicalInitVarType->isPointerType()) {
       // If the initializer and the variable are both pointers check if the
       // un-qualified pointee types match, otherwise we don't use auto.
       if (!Context->hasSameUnqualifiedType(
               CanonicalBeginType->getPointeeType(),
               CanonicalInitVarType->getPointeeType()))
         return false;
-    } else if (!Context->hasSameType(CanonicalInitVarType,
-                                     CanonicalBeginType)) {
-      // Check for qualified types to avoid conversions from non-const to const
-      // iterator types.
-      return false;
     }
   } else if (FixerKind == LFK_PseudoArray) {
     // This call is required to obtain the container.
     const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
     if (!EndCall || !dyn_cast<MemberExpr>(EndCall->getCallee()))
       return false;
   }
   return true;

docs/clang-tidy/checks/modernize-loop-convert.rst

for (vector<int>::iterator it = vec.begin(), e = vec.end(); it != e; ++it) {
     if (!flag) {
       // Accessing and modifying the container is considered risky, but the risk
       // level is not raised here.
       ptr->push_back(*it);
       ref.push_back(*it);
       flag = true;
     }
   }
+
+OpenMP
+^^^^^^
+
+As range-based for loops are only available since OpenMP 5, this check should
+not been used on code with a compatibility requirements of OpenMP prior to
+version 5. It is **intentional** that this check does not make any attempts to
+exclude incorrect diagnostics on OpenMP for loops prior to OpenMP 5.
+
+To prevent this check to be applied (and to break) OpenMP for loops but still be
+applied to non-OpenMP for loops the usage of ``NOLINT`` (see
+:ref:`clang-tidy-nolint`) on the specific for loops is recommended.

docs/clang-tidy/index.rst

configuration can be inspected using -dump-config:
       HeaderFilterRegex: ''
       FormatStyle:     none
       User:            user
       CheckOptions:
         - key:             some-check.SomeOption
           value:           'some value'
       ...
 
+.. _clang-tidy-nolint:
+
 Suppressing Undesired Diagnostics
 =================================
 
 :program:`clang-tidy` diagnostics are intended to call out code that does not
 adhere to a coding standard, or is otherwise problematic in some way.  However,
 if the code is known to be correct, it may be useful to silence the warning.
 Some clang-tidy checks provide a check-specific way to silence the diagnostics,
 e.g.  `bugprone-use-after-move <checks/bugprone-use-after-move.html>`_ can be

test/clang-tidy/modernize-loop-convert-basic.cpp

for (PtrSet<int *>::iterator I = Val_int_ptrs.begin(),
                                  E = Val_int_ptrs.end();
          I != E; ++I) {
       (void) *I;
     }
     // CHECK-MESSAGES: :[[@LINE-5]]:5: warning: use range-based for loop instead
     // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
     TypedefDerefContainer<int> Int_ptrs;
     for (TypedefDerefContainer<int>::iterator I = Int_ptrs.begin(),
                                               E = Int_ptrs.end();
          I != E; ++I) {
       (void) *I;
void different_type() {
   S *Ps;
   for (S::const_iterator It = Ps->begin(), E = Ps->end(); It != E; ++It) {
     printf("s has value %d\n", (*It).X);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent<int> V;
   for (dependent<int>::const_iterator It = V.begin(), E = V.end();
        It != E; ++It) {
     printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent<int>::const_iterator It(V.begin()), E = V.end();
        It != E; ++It) {
     printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
 // If member calls are made to 'this' within the loop, the transform becomes
 // risky as these calls may affect state that affects the loop.
 class C {
 public:
   typedef MutableVal *iterator;

test/clang-tidy/modernize-loop-convert-extra.cpp

void different_type() {
   S *Ps;
   for (S::const_iterator It = Ps->begin(); It != Ps->end(); ++It) {
     printf("s has value %d\n", (*It).X);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent<int> V;
   for (dependent<int>::const_iterator It = V.begin(); It != V.end(); ++It) {
     printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent<int>::const_iterator It(V.begin()); It != V.end(); ++It) {
     printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
 
 
 namespace Macros {
 
 #define TWO_PARAM(x, y) if (x == y) {}
void iterators() {
   // CHECK-FIXES-NEXT: auto H1 = [&I]() { int R = I; };
 
   for (dependent<int>::iterator I = Dep.begin(), E = Dep.end(); I != E; ++I)
     auto H2 = [&]() { int R = *I + 2; };
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent<int>::const_iterator I = Dep.begin(), E = Dep.end();
        I != E; ++I)
     auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = [&I]() { int R = I; };
 
   for (dependent<int>::const_iterator I = Dep.begin(), E = Dep.end();
        I != E; ++I)
     auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent<int>::const_iterator I = Dep.begin(), E = Dep.end();
        I != E; ++I)
     auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
   // When the index is captured by value, we should replace this by a capture
   // by reference. This avoids extra copies.
   // FIXME: this could change semantics on array or pseudoarray loops if the
   // container is captured by copy.
   const int N = 10;