Page MenuHomePhabricator

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

Authored by torbjoernk on Sat, May 11, 12:18 PM.

Details

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

Diff Detail

Event Timeline

torbjoernk created this revision.Sat, May 11, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 11, 12:18 PM
hintonda added inline comments.Sat, May 11, 12:27 PM
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  }

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..

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.

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 added inline comments.Sun, May 12, 12:47 PM
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?

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 added a comment.EditedSun, May 12, 12:59 PM

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.

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.

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?

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.

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.

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)

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?

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.

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.

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 edited the summary of this revision. (Show Details)

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

torbjoernk marked 4 inline comments as done.Mon, May 13, 1:53 PM

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 accepted this revision.Mon, May 13, 2:24 PM

Awesome, thanks...

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

This revision is now accepted and ready to land.Mon, May 13, 2:24 PM
torbjoernk edited the summary of this revision. (Show Details)

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.

torbjoernk marked an inline comment as done.Tue, May 14, 11:09 AM

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.

minor rewording

torbjoernk marked an inline comment as done.Tue, May 14, 12:48 PM
This revision was automatically updated to reflect the committed changes.