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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Could you run the check on llvm sources and post a summary of results here?

A few more inline comments.



================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:102
+  if (std::unique_ptr<CFG> TheCFG =
+          CFG::buildCFG(nullptr, FunctionBody, &ASTCtx, Options))
+    Sequence = llvm::make_unique<ExprSequence>(TheCFG.get(), &ASTCtx);
----------------
What does `nullptr` mean here? Please add an argument comment (`/*ArgumentName=*/nullptr, ...`).


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:109-112
+  const auto *ContainingLambda =
+      Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
+  const auto *ContainingFunc =
+      Result.Nodes.getNodeAs<FunctionDecl>("containing-func");
----------------
nit: Let's put these variable definitions into the corresponding `if` conditions to limit their scope. I'd also move the `if`s to the start of the function.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:122-123
+    FunctionBody = ContainingFunc->getBody();
+  else
+    return;
+
----------------
Instead I'd check that FunctionBody is not nullptr.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:148
+    // (excluding the init stmt).
+    if (const auto ForLoop = dyn_cast<ForStmt>(LoopStmt)) {
+      if (ForLoop->getInc())
----------------
nit: `const auto *`


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:150
+      if (ForLoop->getInc())
+        Match = match(potentiallyModifyVarStmt(CondVar), *ForLoop->getInc(),
+                      ASTCtx);
----------------
Any reason to store the result of the matching instead of returning early? Same below. Please also move the definition of the Match variable to where it's actually needed first time.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:175
+
+    for (const auto &ES : Match) {
+      if (Sequence->potentiallyAfter(LoopStmt, ES.getNodeAs<Stmt>("escStmt")))
----------------
Please use the specific type instead of `auto`.


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.h:37
+private:
+  bool updateSequence(Stmt *FunctionBody, ASTContext &ASTCtx);
+  const Stmt *PrevFunctionBody;
----------------
You seem to have forgotten to update the header.


https://reviews.llvm.org/D40937