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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:12
+the loop condition is not changed. This check detects such loops. A loop is
+considered as infinite if it does not have any loop exit statement (``break``,
+``continue``, ``goto``, ``return``, ``throw`` or a call to a function called as
----------------
s/as//


================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:156
+  }
+  int &ii = i;
+}
----------------
baloghadamsoftware wrote:
> gribozavr wrote:
> > 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;
> > }
> > ```
> You are right, but how frequent are such cases? I found no false positives at all when checking some ope-source projects. Should we spend resources to detect escaping in a nesting loop? I could not find a cheap way yet. I suppose this can only be done when nesting two loops. (I am sure we can ignore the cases where the outer loop is implemented using a `goto`.
I don't know, however, this is one of the few sources of false positives for this check. This check generally can't detect all infinite loops, it is only trying to detect obvious ones. Therefore, I don't think it is exchange a bit more precision for false positives.

My best advice is to drop the heuristic about variable escaping after the loop. Any escaping => silence the warning.

If you want to handle this case correctly, you have to use the CFG.


================
Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:225
+  while (1)
+    ; //FIXME: We expect report in this case.
+}
----------------
baloghadamsoftware wrote:
> gribozavr wrote:
> > 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.
> We already handle loop exit statements including calls to `[[noreturn]]` functions. Is that not enough?
User-defined functions are not always annotated with ``[[noreturn]]``, and sometimes can't be annotated, for example, because they call `exit()` conditionally:

```
while (true) {
  std::string command = readCommand();
  executeCommand(command);
}

void executeCommand(const std::string &command) {
  if (command == "exit") {
    exit();
  }
  ...
}
```


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

https://reviews.llvm.org/D64736