[clang-tidy] terminating continue check

[koldaniel]

Summary

checks if a 'continue' statement terminates the loop

[alexfh]

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?

[koldaniel]

In D33844#798900, @alexfh wrote:

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

[JonasToth]

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

20

I think you can make one sentence out of both.

docs/clang-tidy/checks/misc-terminating-continue.rst

7

Maybe rephrase a little:

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

22

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

[• Quuxplusone]

clang-tidy/misc/TerminatingContinueCheck.cpp

43

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

8

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]

test/clang-tidy/misc-terminating-continue.cpp

8

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]

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

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

30–33

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

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

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

[alexfh]

There are still outstanding comments.

[koldaniel]

In D33844#1086565, @alexfh wrote:

Which comments do you think are still relevant?

[alexfh]

In D33844#1095862, @koldaniel wrote:

In D33844#1086565, @alexfh wrote:

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?

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
+  TerminatingContinueCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NonCopyableObjects.cpp
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NonCopyableObjects.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
+#include "TerminatingContinueCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
+    CheckFactories.registerCheck<TerminatingContinueCheck>(
+        "misc-terminating-continue");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");

clang-tidy/misc/TerminatingContinueCheck.h

+//===--- TerminatingContinueCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_TERMINATING_CONTINUE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_TERMINATING_CONTINUE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Checks if a 'continue' statement terminates the loop (i.e. the loop has
+/// a condition which always evaluates to false).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-terminating-continue.html
+class TerminatingContinueCheck : public ClangTidyCheck {
+public:
+  TerminatingContinueCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_TERMINATING_CONTINUE_H

clang-tidy/misc/TerminatingContinueCheck.cpp

+//===--- TerminatingContinueCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TerminatingContinueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void TerminatingContinueCheck::registerMatchers(MatchFinder *Finder) {
+  const auto doWithFalse =
+      doStmt(hasCondition(ignoringImpCasts(
+                 anyOf(cxxBoolLiteral(equals(false)), integerLiteral(equals(0)),
+                       cxxNullPtrLiteralExpr(), gnuNullExpr()))),
+             equalsBoundNode("closestLoop"))
+          .bind("doWithFalseCond");
+
+  Finder->addMatcher(
+      continueStmt(anyOf(hasAncestor(forStmt().bind("closestLoop")),
+                         hasAncestor(cxxForRangeStmt().bind("closestLoop")),
+                         hasAncestor(whileStmt().bind("closestLoop")),
+                         hasAncestor(doStmt().bind("closestLoop"))),
+                   hasAncestor(doWithFalse))
+          .bind("continue"),
+      this);
+}
+
+void TerminatingContinueCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ContStmt = Result.Nodes.getNodeAs<ContinueStmt>("continue");
+
+  auto Diag =
+      diag(ContStmt->getLocStart(),
+           "'continue' in loop with false condition is equivalent to 'break'");
+  Diag << FixItHint::CreateReplacement(ContStmt->getSourceRange(), "break");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-namespace-comment
    llvm-twine-local
    misc-definitions-in-headers
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects
    misc-redundant-expression
    misc-static-assert
+   misc-terminating-continue
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-using-decls
    modernize-avoid-bind
    modernize-deprecated-headers

docs/clang-tidy/checks/misc-terminating-continue.rst

+.. title:: clang-tidy - misc-terminating-continue
+
+misc-terminating-continue
+=========================
+
+Detects `do while` loops with a condition always evaluating to false that
+have a `continue` statement, as this `continue` terminates the loop
+effectively.
+
+.. code-block:: c++
+
+  Parser::TPResult Parser::TryParseProtocolQualifiers() {
+    assert(Tok.is(tok::less) && "Expected '<' for qualifier list");
+    ConsumeToken();
+    do {
+      if (Tok.isNot(tok::identifier))
+        return TPResult::Error;
+      ConsumeToken();
+
+      if (Tok.is(tok::comma)) {
+        ConsumeToken();
+
+        //Terminating continue
+        continue;
+      }
+
+      if (Tok.is(tok::greater)) {
+        ConsumeToken();
+        return TPResult::Ambiguous;
+      }
+    } while (false);
+
+    return TPResult::Error;
+  }

test/clang-tidy/misc-terminating-continue.cpp

+// RUN: %check_clang_tidy %s misc-terminating-continue %t
+
+void f() {
+  do {
+    continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'continue' in loop with false condition is equivalent to 'break' [misc-terminating-continue]
+    // CHECK-FIXES: break;
+  } while(false);
+
+  do {
+    continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'continue' in loop with false condition is equivalent to 'break' [misc-terminating-continue]
+    // CHECK-FIXES: break;
+  } while(0);
+
+  do {
+    continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'continue' in loop with false condition is equivalent to 'break' [misc-terminating-continue]
+    // CHECK-FIXES: break;
+  } while(nullptr);
+
+  do {
+    continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'continue' in loop with false condition is equivalent to 'break' [misc-terminating-continue]
+    // CHECK-FIXES: break;
+  } while(__null);
+
+
+  do {
+    int x = 1;
+    if (x > 0) continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'continue' in loop with false condition is equivalent to 'break' [misc-terminating-continue]
+    // CHECK-FIXES: if (x > 0) break;
+  } while (false);
+}
+
+void g() {
+  do {
+    do {
+      continue;
+      int x = 1;
+    } while (1 == 1);
+  } while (false);
+
+  do {
+    for (int i = 0; i < 1; ++i) {
+      continue;
+      int x = 1;
+    }
+  } while (false);
+
+  do {
+    while (true) {
+      continue;
+      int x = 1;
+    }
+  } while (false);
+
+  int v[] = {1,2,3,34};
+  do {
+    for (int n : v) {
+      if (n>2) continue;
+    }
+  } while (false);
+}