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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
