[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:94-99
+  std::unique_ptr<CFG> TheCFG =
+      CFG::buildCFG(nullptr, FunctionBody, &ASTCtx, Options);
+  if (!TheCFG)
+    return std::unique_ptr<ExprSequence>();
+
+  return llvm::make_unique<ExprSequence>(TheCFG.get(), &ASTCtx);
----------------
A little suggestion:

  if (std::unique_ptr<CFG> TheCFG = ...)
    return llvm::make_unique<ExprSequence>(TheCFG.get(), &ASTCtx);
  return {};


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:128
+    return;
+  std::unique_ptr<ExprSequence> Sequence = createSequence(FunctionBody, ASTCtx);
+
----------------
The main concern I have now is that the check may end up creating the CFG multiple times for the same function, which may be rather expensive in certain cases (consider a large function consisting of loops that trigger the matcher). It's hard to predict how common is this situation in real code. Do you see how this could be restructured to avoid unnecessary work?


================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:179
+  }
+  CondVarNames.resize(CondVarNames.size() - 2);
+
----------------
I suggest a more straightforward version:

​  std::string CondVarNames;
  for (const auto &CVM : CondVarMatches) {
    if (!CondVarNames.empty())
      CondVarNames.append(", ");
    CondVarNames.append(CVM.getNodeAs<VarDecl>("condvar")->getNameAsString());
​  }



https://reviews.llvm.org/D40937