[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

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]

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]

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]

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]

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/bugprone/BugproneTidyModule.cpp

 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
+#include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "bugprone-suspicious-missing-comma");
     CheckFactories.registerCheck<SuspiciousSemicolonCheck>(
         "bugprone-suspicious-semicolon");
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "bugprone-suspicious-string-compare");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
+    CheckFactories.registerCheck<TerminatingContinueCheck>(
+        "bugprone-terminating-continue");
     CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
         "bugprone-throw-keyword-missing");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnusedRaiiCheck>(
         "bugprone-unused-raii");

clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
+  TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 

clang-tidy/bugprone/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_BUGPRONE_TERMINATINGCONTINUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TERMINATINGCONTINUECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// 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/bugprone-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 bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TERMINATINGCONTINUECHECK_H

clang-tidy/bugprone/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"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void TerminatingContinueCheck::registerMatchers(MatchFinder *Finder) {
+  const auto doWithFalse =
+      doStmt(hasCondition(ignoringImpCasts(
+                 anyOf(cxxBoolLiteral(equals(false)), integerLiteral(equals(0)),
+                       cxxNullPtrLiteralExpr(), gnuNullExpr()))),
+             equalsBoundNode("closestLoop"));
+
+  Finder->addMatcher(
+      continueStmt(hasAncestor(stmt(anyOf(forStmt(), whileStmt(),
+                                          cxxForRangeStmt(), 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'")
+      << tooling::fixit::createReplacement(*ContStmt, "break");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New :doc:`modernize-use-uncaught-exceptions
   ``std::uncaught_exceptions``.
 
 - New :doc:`portability-simd-intrinsics
   <clang-tidy/checks/portability-simd-intrinsics>` check
 
   Warns or suggests alternatives if SIMD intrinsics are used which can be replaced by
   ``std::experimental::simd`` operations.
 
+- New :doc:`bugprone-terminating-continue
+  <clang-tidy/checks/bugprone-terminating-continue>` check
+
+  Checks if a ``continue`` statement terminates the loop.
+
 - New :doc:`zircon-temporary-objects
   <clang-tidy/checks/zircon-temporary-objects>` check
 
   Warns on construction of specific temporary objects in the Zircon kernel.
 
 - New alias :doc:`hicpp-avoid-goto
   <clang-tidy/checks/hicpp-avoid-goto>` to :doc:`cppcoreguidelines-avoid-goto
   <clang-tidy/checks/cppcoreguidelines-avoid-goto>`

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

+.. title:: clang-tidy - bugprone-terminating-continue
+
+bugprone-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++
+
+  void f() {
+  do {
+    // some code
+    continue; // terminating continue
+    // some other code
+  } while(false);

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-string-integer-assignment
    bugprone-string-literal-with-embedded-nul
    bugprone-suspicious-enum-usage
    bugprone-suspicious-memset-usage
    bugprone-suspicious-missing-comma
    bugprone-suspicious-semicolon
    bugprone-suspicious-string-compare
    bugprone-swapped-arguments
+   bugprone-terminating-continue
    bugprone-throw-keyword-missing
    bugprone-undefined-memory-manipulation
    bugprone-undelegated-constructor
    bugprone-unused-raii
    bugprone-unused-return-value
    bugprone-use-after-move
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>

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

+// RUN: %check_clang_tidy %s bugprone-terminating-continue %t
+
+void f() {
+  do {
+    continue;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'continue' in loop with false condition is equivalent to 'break' [bugprone-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' [bugprone-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' [bugprone-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' [bugprone-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' [bugprone-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);
+}