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