[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:23-25
+static internal::Matcher<Stmt> loopEndingStmt() {
+  return stmt(anyOf(breakStmt(), returnStmt(), gotoStmt(), cxxThrowExpr()));
+}
----------------
This doesn't have to be a function. It can be a local variable or can be inlined into the callsite.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:32
+void InfiniteLoopCheck::registerMatchers(MatchFinder *Finder) {
+  const auto loopCondition = allOf(hasCondition(expr().bind("condition")),
+              anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
----------------
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:105
+  return llvm::make_unique<ExprSequence>(
+      *(new ExprSequence(TheCFG.get(), &ASTCtx)));
+}
----------------
Why not just `llvm::make_unique<ExprSequence>(TheCFG.get(), &ASTCtx)`?

It also may be that you somehow lost a bunch of fixes, since this comment has already been addressed at some point: https://reviews.llvm.org/D40937?id=125870#inline-357357


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:186
+  diag(LoopStmt->getLocStart(),
+       "%plural{1:The condition variable|:None of the condition variables}0 "
+       "(%1) %plural{1:is not|:are}0 updated in the loop body")
----------------
Clang diagnostics (and clang-tidy warnings) are not complete sentences and shouldn't start with a capital letter.


https://reviews.llvm.org/D40937