baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp:186
+ }
+ }
+
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp:202
+ }
+ }
+
----------------
gribozavr2 wrote:
> I think this logic should go into `getCondVarNames`. It even seems like the
> existing logic should work without changes, it is walking all children of a
> provided statement.
The same is valid here that I wrote above. `getCondVarNames()` did not grab the
variable names in the initialization expression of the variable declared inside
the condition part of the `while` loop because they are no children of the
`Condition` part in Clang. You can see this in the bugzilla bug report as well.
There the variable `pattern` did not appear in the diagnostic message.
Repository:
rCTE Clang Tools Extra
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
