[PATCH] D33844: [clang-tidy] terminating continue check

[Alexander Kornienko via Phabricator via cfe-commits]

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