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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 3 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp:186
+    }
+  }
+
----------------
gribozavr2 wrote:
> 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.
Oh, I forgot that the declared variable gets a new value at every iteration thus its changes are not relevant. Thank you, I fixed this now.


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

https://reviews.llvm.org/D73270