I wrote the original for loop warning. I did try to extend it to while loops, but due to the different usage of these types of loops, there was a higher false positive rate for while loops that in for loops. For loops are generally self-contained, so checking only the loop and loop body is a good way to determine if the condition is changed. However, while loops are also used in things like multi-threaded code and the while loop is used as a wait.

This seems to be acting weird in template instantations. Here's an example where the lambda only errors inside a template.

This was causing some issues in tests.
I've reverted this in 1150f02c77b81adca4d0c67afdef23321e93db89
and sent @dvyukov details to reproduce the problem I saw.

In D108794#2978134, @dblaikie wrote:

Ping

In D108794#2976063, @rtrieu wrote:

It looks like a strict improvement on printing and most callers using the default args won't need to be updated.

There's one more function call that should be updated:
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/DumpAST.cpp#L298

Fixing that and the comment and this should be good to go in.

Ah, thanks for catching that (just running check-clang doesn't catch this, and it doesn't seem like there's a check-clang-tools-extra - any idea if there's something narrower than check-all that would run clang-tools-extra tests?). Updated that caller to preserve the existing unqualified behavior.

It looks like a strict improvement on printing and most callers using the default args won't need to be updated.

Thanks for the quick fix. I've verified that it fixes my full test case.

I ran across a llvm_unreachable that points to this commit. Repro instructions below:

In D101387#2720500, @MaskRay wrote:

@rtrieu Do we have a way appending arbitrary messages to a diagnostic template?

LGTM

LGTM

In D85287#2199490, @sberg wrote:

In D85287#2199463, @rtrieu wrote:

Also, have you tried running warning over a codebase?

As I wrote: "At least building LibreOffice with this change caused no false positives." (Or maybe I misunderstand your question.)

I looked back on the commits and while I did commit, I did it on the behalf of Anders Rönnholm, who did not have commit access at the time. I haven't seen activity from Anders in a while, but added to subscribers just in case.

LGTM

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

Heads up in case it affects you refactoring work:
While looking through this code, I found a bug I previously made. I fixed it with a small change, but that lies in the middle of your refactoring during FieldDecl handling. The change is here:

In D71503#1784697, @lebedev.ri wrote:

Thank you for working on this!
This seems to be missing tests.

In D69292#1747062, @mclow.lists wrote:

Sorry I'm late to the party; I've been traveling for 3+ weeks.
I would like to be reassured that the following code will not warn:

`
long foo = ...; // some calculation
if (foo < std::numeric_limits<int>::min() || foo > std::numeric_limits<int>::max()) .....

This is important for systems where sizeof(int) == sizeof(long)

Add -Wall tests to check that certain warning groups are active with it and a test to check all warning groups that are turned on by -Wall.

The recommited patch is hitting an assert. See message and reduced test case below:

In D69292#1718415, @thakis wrote:

Abstractly this lgtm. Do you have any data on true / false positive rates?

In D66046#1717229, @thakis wrote:

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

In D66046#1696785, @xbolva00 wrote:

Could this patch solve https://bugs.llvm.org/show_bug.cgi?id=43573?

Add more test cases and split new warnings into a separate warning group, but still under -Wparentheses

I have reverted this in r370006.

In D66044#1626008, @NoQ wrote:

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

Better handling of arrays in self compares.

Update comments to explain why bitwise-xor is treated as a logical operator.

Check array accesses.