[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

Fri Jul 19 09:55:51 PDT 2019

gribozavr added inline comments.

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:25
+
+static bool isAccessForVar(const Stmt *St, const VarDecl *Var) {
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(St))
----------------
St => S

"S" is a common abbreviation in the Clang codebase, "St" isn't.

(everywhere in the patch)

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:49
+
+static bool hasPtrOrReferenceBefore(const Stmt *St, const Stmt *LoopStmt,
+                                    const VarDecl *Var) {
----------------
Please add documentation comments.

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:92
+                                       ASTContext *Context) {
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) {
+    if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl())) {
----------------
Please separate the logic that finds variables from the rest of the logic.

Finding variables has to be recursive, the rest does not have to be.

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:160
+      allOf(hasCondition(expr().bind("condition")),
+            unless(hasBody(hasDescendant(loopEndingStmt()))));
+
----------------
hasDescendant will recurse into other functions defined in the loop body, for example into lambdas.

```
for (...) {
  auto x = []() { throw 0; }
}
```

You can add the forFunction matcher to the loop ending statement matcher, and then use equalsNode to confirm that the loop ending statement is in the same function as the for loop.

Please also add tests for this case.

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:181
+
+  diag(getFirstCondVar(Cond)->getLocation(),
+       "None of the condition variables "
----------------
I'd suggest to attach the diagnostic to the loop -- the problem is with the loop, not with the first reference to the variable.

================
Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:182
+  diag(getFirstCondVar(Cond)->getLocation(),
+       "None of the condition variables "
+       "(%0) are updated in the loop body")
----------------
ClangTidy messages are sentence fragments that start with a lowercase letter.

It would be also helpful to state the problem explicitly.

"this loop is infinite; none of its condition variables are updated in the loop body"

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:6
+
+Checks for obvious infinite loops (loops where the condition variable is not
+changed at all).
----------------
s/checks for/finds/ (everywhere in the patch)

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:9
+
+Detecting infinite loops by a finite program is well known to be impossible
+(Halting-problem). However there are some simple but common cases where it
----------------
"Finding infinite loops is well-known to be impossible (halting problem)."

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:10
+Detecting infinite loops by a finite program is well known to be impossible
+(Halting-problem). However there are some simple but common cases where it
+is possible: the loop condition is not changed in the loop at all. This check
----------------
"However, it is possible to detect some obvious infinite loops, for example, if the loop condition is not changed."

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:16
+
+- It is a local variable of the function.
+- It has no reference or pointer aliases before or inside the loop.
----------------
"It is a local variable" (local variables can only be declared in functions)

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:18
+- It has no reference or pointer aliases before or inside the loop.
+- It is no member expression.
+
----------------
I don't quite understand what you mean by "it is no member expression". Also, users likely won't understand the term "member expression".

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:23
+
+For example, the following loop is considered as infinite since mistakenly
+`j` is incremented instead of `i`:
----------------
s/considered as infinite/considered infinite/

================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:24
+For example, the following loop is considered as infinite since mistakenly
+`j` is incremented instead of `i`:
+
----------------
I don't think we can say the "mistakenly" part. Explaining checker's behavior like that suggests that the checker detects the user's intent, which it does not. For example, it could also be the case that the user wanted to increment both i and j in the loop body.

All we can say is the loop is infinite because in the loop condition "i < 10" all variables (i) never change.

================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:43
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+    Limit--;
----------------
Please add a period at the end of the comment.

Please also put comments on separate lines to avoid awkward wrapping like in tests below (such complex wrapping also discourages people from writing more detailed comments.)

================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:59
+
+int any_function();
+
----------------
"unknown_function"?

================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:108
+
+void escape_inside1() {
+  int i = 0;
----------------
Please also add tests for lambdas capturing the loop variable by reference.

================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:156
+  }
+  int &ii = i;
+}
----------------
Is all escaping that syntactically follows the loop actually irrelevant? For example:

```
int i = 0;
int j = 0;
int *p = &j;
for (int k = 0; k < 5; k++) {
  *p = 100;
  if (k != 0) {
    // This loop would be reported as infinite.
    while (i < 10) {
    }
  }
  p = &i;
}
```

================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:225
+  while (1)
+    ; //FIXME: We expect report in this case.
+}
----------------
Why?

Intentional infinite loops *that perform side-effects in their body* are quite common -- for example, network servers, command-line interpreters etc.

Also, if the loop body calls an arbitrary function, we don't know if it will throw, or `exit()`, or kill the current thread or whatnot.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64736/new/

https://reviews.llvm.org/D64736