[PATCH] D33844: [clang-tidy] terminating continue check
This revision was automatically updated to reflect the committed changes. Closed by commit rL332223: [clang-tidy] Add terminating continue check (authored by xazax, committed by ). Herald added subscribers: llvm-commits, klimek. Changed prior to commit: https://reviews.llvm.org/D33844?vs=142793=146566#toc Repository: rL LLVM https://reviews.llvm.org/D33844 Files: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt clang-tools-extra/trunk/clang-tidy/bugprone/TerminatingContinueCheck.cpp clang-tools-extra/trunk/clang-tidy/bugprone/TerminatingContinueCheck.h clang-tools-extra/trunk/docs/ReleaseNotes.rst clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-terminating-continue.rst clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst clang-tools-extra/trunk/test/clang-tidy/bugprone-terminating-continue.cpp Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-terminating-continue.cpp === --- clang-tools-extra/trunk/test/clang-tidy/bugprone-terminating-continue.cpp +++ clang-tools-extra/trunk/test/clang-tidy/bugprone-terminating-continue.cpp @@ -0,0 +1,65 @@ +// 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); +} Index: clang-tools-extra/trunk/clang-tidy/bugprone/TerminatingContinueCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/bugprone/TerminatingContinueCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/bugprone/TerminatingContinueCheck.cpp @@ -0,0 +1,49 @@ +//===--- 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 ) { + const auto *ContStmt = Result.Nodes.getNodeAs("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 Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
[PATCH] D33844: [clang-tidy] terminating continue check
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D33844#1095862, @koldaniel wrote: > In https://reviews.llvm.org/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? https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
koldaniel added a comment. In https://reviews.llvm.org/D33844#1086565, @alexfh wrote: > Which comments do you think are still relevant? https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. There are still outstanding comments. https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
koldaniel updated this revision to Diff 142793. https://reviews.llvm.org/D33844 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/TerminatingContinueCheck.cpp clang-tidy/bugprone/TerminatingContinueCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/bugprone-terminating-continue.rst docs/clang-tidy/checks/list.rst test/clang-tidy/bugprone-terminating-continue.cpp Index: test/clang-tidy/bugprone-terminating-continue.cpp === --- /dev/null +++ test/clang-tidy/bugprone-terminating-continue.cpp @@ -0,0 +1,65 @@ +// 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); +} Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -48,6 +48,7 @@ bugprone-suspicious-semicolon bugprone-suspicious-string-compare bugprone-swapped-arguments + bugprone-terminating-continue bugprone-throw-keyword-missing bugprone-undefined-memory-manipulation bugprone-undelegated-constructor Index: docs/clang-tidy/checks/bugprone-terminating-continue.rst === --- /dev/null +++ docs/clang-tidy/checks/bugprone-terminating-continue.rst @@ -0,0 +1,17 @@ +.. 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); Index: docs/ReleaseNotes.rst === --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -130,6 +130,11 @@ Warns or suggests alternatives if SIMD intrinsics are used which can be replaced by ``std::experimental::simd`` operations. +- New :doc:`bugprone-terminating-continue + ` check + + Checks if a ``continue`` statement terminates the loop. + - New :doc:`zircon-temporary-objects ` check Index: clang-tidy/bugprone/TerminatingContinueCheck.h === --- /dev/null +++ clang-tidy/bugprone/TerminatingContinueCheck.h @@ -0,0 +1,36 @@ +//===--- 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: +
[PATCH] D33844: [clang-tidy] terminating continue check
alexfh added a comment. 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). Comment at: clang-tidy/misc/TerminatingContinueCheck.cpp:27 + equalsBoundNode("closestLoop")) + .bind("doWithFalseCond"); + The "doWithFalseCond" binding isn't used and can be removed. Comment at: clang-tidy/misc/TerminatingContinueCheck.cpp:30-33 + continueStmt(anyOf(hasAncestor(forStmt().bind("closestLoop")), + hasAncestor(cxxForRangeStmt().bind("closestLoop")), + hasAncestor(whileStmt().bind("closestLoop")), + hasAncestor(doStmt().bind("closestLoop"))), 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)) Comment at: clang-tidy/misc/TerminatingContinueCheck.cpp:45 + "'continue' in loop with false condition is equivalent to 'break'"); + Diag << FixItHint::CreateReplacement(ContStmt->getSourceRange(), "break"); +} 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");` Comment at: docs/clang-tidy/checks/misc-terminating-continue.rst:12-21 + Parser::TPResult Parser::TryParseProtocolQualifiers() { +assert(Tok.is(tok::less) && "Expected '<' for qualifier list"); +ConsumeToken(); +do { + if (Tok.isNot(tok::identifier)) +return TPResult::Error; + ConsumeToken(); Quoting a whole method from Clang isn't necessary for the purpose of this documentation. I'd go with a simpler example. https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
JonasToth added inline comments. Comment at: test/clang-tidy/misc-terminating-continue.cpp:7 +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: terminating 'continue' [misc-terminating-continue] + } while(false); + Quuxplusone wrote: > 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. 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` https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
koldaniel updated this revision to Diff 139953. https://reviews.llvm.org/D33844 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/TerminatingContinueCheck.cpp clang-tidy/misc/TerminatingContinueCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-terminating-continue.rst test/clang-tidy/misc-terminating-continue.cpp Index: test/clang-tidy/misc-terminating-continue.cpp === --- /dev/null +++ test/clang-tidy/misc-terminating-continue.cpp @@ -0,0 +1,65 @@ +// 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); +} Index: docs/clang-tidy/checks/misc-terminating-continue.rst === --- /dev/null +++ docs/clang-tidy/checks/misc-terminating-continue.rst @@ -0,0 +1,34 @@ +.. 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; + } Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -149,6 +149,7 @@ 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 Index: clang-tidy/misc/TerminatingContinueCheck.h === --- /dev/null +++ clang-tidy/misc/TerminatingContinueCheck.h @@ -0,0 +1,36 @@ +//===--- 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
[PATCH] D33844: [clang-tidy] terminating continue check
Quuxplusone added inline comments. Comment at: clang-tidy/misc/TerminatingContinueCheck.cpp:42 + + auto Diag = diag(ContStmt->getLocStart(), "terminating 'continue'"); + Diag << FixItHint::CreateReplacement(ContStmt->getSourceRange(), "break"); 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. :) Comment at: test/clang-tidy/misc-terminating-continue.cpp:7 +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: terminating 'continue' [misc-terminating-continue] + } while(false); + 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33844: [clang-tidy] terminating continue check
JonasToth added a comment. 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? Comment at: clang-tidy/misc/TerminatingContinueCheck.h:19 + +/// Checks if a 'continue' statement terminates the loop. It does if the loop +/// has false condition. I think you can make one sentence out of both. Comment at: docs/clang-tidy/checks/misc-terminating-continue.rst:6 + +Detects `do while` loops with `false` conditions that have `continue` statement +as this `continue` terminates the loop effectively. Maybe rephrase a little: `loops with a condition always evaluating to false` or something like it. Comment at: docs/clang-tidy/checks/misc-terminating-continue.rst:21 +ConsumeToken(); +continue; + } Please emphasize this location, so that it is absolutly clear what you mean. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D33844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits