This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] terminating continue check
ClosedPublic

Authored by koldaniel on Jun 2 2017, 11:02 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

SilverGeri created this revision.Jun 2 2017, 11:02 AM
alexfh edited edge metadata.Jul 4 2017, 7:03 AM
alexfh added a subscriber: cfe-commits.

Please don't forget to add cfe-commits to Subscribers: when initially sending the patch for review. Otherwise nobody on the list will see it.

Could you run the check on LLVM+Clang and post a summary of your findings here?

alexfh requested changes to this revision.Jul 4 2017, 7:05 AM
This revision now requires changes to proceed.Jul 4 2017, 7:05 AM
koldaniel commandeered this revision.Mar 12 2018, 8:32 AM
koldaniel added a reviewer: SilverGeri.

Please don't forget to add cfe-commits to Subscribers: when initially sending the patch for review. Otherwise nobody on the list will see it.

Could you run the check on LLVM+Clang and post a summary of your findings here?

Hi,

Result of a clang-tidy run on LLVM+Clang with only misc-terminating-continue turned on:

1 Warning:
llvm/tools/clang/lib/Parse/ParseTentative.cpp:1678:7: warning: terminating 'continue' [misc-terminating-continue]

continue;
^~~~~~~~~
break
lebedev.ri retitled this revision from terminating continue check to [clang-tidy] terminating continue check.Mar 22 2018, 7:57 AM
lebedev.ri set the repository for this revision to rCTE Clang Tools Extra.
lebedev.ri added a project: Restricted Project.

I think this check could land in the bugprone module.

Given this situation won't appear a lot in codebases, did you check other codebases than LLVM?

clang-tidy/misc/TerminatingContinueCheck.h
19 ↗(On Diff #101239)

I think you can make one sentence out of both.

docs/clang-tidy/checks/misc-terminating-continue.rst
6 ↗(On Diff #101239)

Maybe rephrase a little:

loops with a condition always evaluating to false or something like it.

21 ↗(On Diff #101239)

Please emphasize this location, so that it is absolutly clear what you mean.

Quuxplusone added inline comments.
clang-tidy/misc/TerminatingContinueCheck.cpp
42 ↗(On Diff #101239)

It was not clear to me what a "terminating 'continue'" was, just from seeing this error message. Wouldn't it make more sense to emit a self-explanatory diagnostic, such as

'continue', in loop with false condition, is equivalent to 'break'

? And then you could even suggest a fixit of replacing 'continue' with 'break'... oh, which you already do. :)

test/clang-tidy/misc-terminating-continue.cpp
7 ↗(On Diff #101239)

I initially expected to see tests also for

goto L1;
while (false) {
    L1:
    continue;  // 'continue' is equivalent to 'break'
}

int v = 0;
goto L1;
for (; false; ++v) {
    L1:
    continue;  // 'continue' is NOT equivalent to 'break'
}

although I assume your current patch doesn't actually diagnose the former, and that's probably fine.

JonasToth added inline comments.Mar 27 2018, 10:42 AM
test/clang-tidy/misc-terminating-continue.cpp
7 ↗(On Diff #101239)

I think both of your examples are out of scope for this check and are they realistic?

The usage of goto is addressed in cppcoreguidelines-avoid-goto

alexfh added a comment.Apr 5 2018, 5:24 AM

Please move the check to bugprone- module. clang-tidy/rename_check.py script should do most of the job (you'll have to remove the unnecessary "renamed check ..." entry in the release notes).

clang-tidy/misc/TerminatingContinueCheck.cpp
27 ↗(On Diff #139953)

The "doWithFalseCond" binding isn't used and can be removed.

30–33 ↗(On Diff #139953)

Can we avoid repeated traversal of ancestors by inverting the order of matchers here? E.g.

continueStmt(hasAncestor(stmt(anyOf(forStmt(), whileStmt(), ...)).bind("closestLoop")), hasAncestor(doWithFalse))
45 ↗(On Diff #139953)

The variable doesn't make the code any better, please remove it. The creation of the replacement could be formulated a bit more succint: tooling::fixit::createReplacement(ContStmt, "break");

docs/clang-tidy/checks/misc-terminating-continue.rst
12–21 ↗(On Diff #139953)

Quoting a whole method from Clang isn't necessary for the purpose of this documentation. I'd go with a simpler example.

alexfh requested changes to this revision.Apr 5 2018, 5:24 AM
This revision now requires changes to proceed.Apr 5 2018, 5:24 AM
alexfh requested changes to this revision.May 3 2018, 9:10 AM

There are still outstanding comments.

This revision now requires changes to proceed.May 3 2018, 9:10 AM
koldaniel added a comment.EditedMay 11 2018, 6:10 AM

Which comments do you think are still relevant?

alexfh accepted this revision.May 11 2018, 7:30 AM

Which comments do you think are still relevant?

Weird. When I was writing the comment, the code looked identical to the version from Mar 27. It may have been something wrong with my browser, with the Phabricator, or with the way I used it. Anyway, looks good now. Sorry for the confusion.

Do you need someone to commit the patch for you?

This revision is now accepted and ready to land.May 11 2018, 7:30 AM
This revision was automatically updated to reflect the committed changes.