[PATCH] D73270: [clang-tidy] Fix false positive in bugprone-infinite-loop

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp:186
+    }
+  }
+
----------------
baloghadamsoftware wrote:
> gribozavr2 wrote:
> > I think this logic should go into `isChanged()`, similarly to how it handles for loops today.
> `isChanged()` looks for changes of a specific variable. This part collects the variables to check. The fundamental problem with the current logic is that it only look for the condition variables in the `Condition` of the loop statement, but it does not contain the initial value of the variable declared inside the condition part of the `while` statement. Thus in case of `while (int m = n) { n--; }` the condition is only the expression `m`. Thus we simply cannot grab `n` by only checking the condition thus consider this loop as infinite because `m` is not changed. Taking the whole loop statement instead of the condition is also wrong because then the following obviously infinite loop is not detected: `while (n) { m--; }` because `m` is also grabbed from the body which must not. We must explicitly check the variable of the initialization expression of variable declared in the condition of `while` loops because it is a separate child of the statement.
Yes, I see what you mean -- and my suggestion was incorrect.

However, right now the variable declared in the while loop is being treated like something that carries over to the next loop iteration:

```
while (int k = Limit) { k--; } // no warning with this patch
```

This is because this while loop still has a condition that checks that `k != 0`, and we scan that in the isAtLeastOneCondVarChanged call on line 173.

I think a better fix would bind `Cond` to the initialization expression of the while loop variable. Like this:

```
void InfiniteLoopCheck::check(const MatchFinder::MatchResult &Result) {
  const auto *Cond = Result.Nodes.getNodeAs<Expr>("condition");
  const auto *LoopStmt = Result.Nodes.getNodeAs<Stmt>("loop-stmt");
  const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");

  bool ShouldHaveConditionVariables = true;
  if (const auto *While = dyn_cast<WhileStmt>(LoopStmt)) {
    if (const VarDecl *LoopVarDecl = While->getConditionVariable()) {
      if (const Expr *Init = LoopVarDecl->getInit()) {
        ShouldHaveConditionVariables = false;
        Cond = Init;
      }
    }
  }
  Cond->dump();

  if (isAtLeastOneCondVarChanged(Func, LoopStmt, Cond, Result.Context)) {
    return;
  }

  std::string CondVarNames = getCondVarNames(Cond);
  if (ShouldHaveConditionVariables && CondVarNames.empty())
    return;

  if (CondVarNames.empty()) {
    diag(LoopStmt->getBeginLoc(),
         "this loop is infinite; it does not check any variables in the condition");
  } else {
    diag(LoopStmt->getBeginLoc(),
         "this loop is infinite; none of its condition variables (%0)"
         " are updated in the loop body")
        << CondVarNames;
  }
}
```

If you agree, please also add `while (int k = Limit) { k--; }` to the tests.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp:69
   }
 }
 
----------------
There seems to be no test coverage for the following case:

```
while (i--) { // no warning
  j--;
}
```

I'd appreciate if you could add that.

And also this one, relevant to the bug you're fixing:

```
while (int k = Limit--) { // no warning
  i--;
}
```


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

https://reviews.llvm.org/D73270