I noticed some build failures after your commit. I'm trying to get a standalone reproducer.

In D133586#3781468, @rmaz wrote:

In D133586#3781381, @rtrieu wrote:

Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record is the same?

Do you have a test case for this?

In D130510#3728719, @aaron.ballman wrote:

In D130510#3727096, @rtrieu wrote:

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

The existing function returns whether the expression is an ICE (sort of), the replacement function calculates the actual value and returns an optional APInt so you can perform operations on it directly. That said, a future refactoring could probably remove IsIntegerLiteralConstantExpr() and replace it with getIntegerLiteralSubexpressionValue(), but the only caller of IsIntegerLiteralConstantExpr() doesn't actually need the value at that point.

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

I think this is a reasonable step for improving compile times. I put some suggestions for more descriptive names below (he said, after suggesting those names in the first place).

void foo(long x) {
  if ((x & 1) == 1L) return;  // bad always false warning here
  static_assert(sizeof(int) < sizeof(long), "long is bigger than int");
  static_assert((long(15) & 1) == 1L, "proof that condition can be true");
}

I found this false positive case when testing your new patch. The condition is fine, but it gives an always false warning. When fixed, this would be another good test case to include.

Can you add my earlier test case or something like it to SemaCXX/warn-bitwise-compare.cpp ?

In D130510#3693494, @aaron.ballman wrote:

In D130510#3692654, @rtrieu wrote:

Because of this, warnings should treat dependent expressions as non-constant even when they can be evaluated, so only b3 should get a warning. This is causing the warning to be emitted on code heavy in template meta-programming, such as array libraries. Please revert or fix.

Yeah, I agree. Unfortunately, the CFG makes this exceptionally difficult because it walks over the instantiated code, so we've lost that the original expression was dependent by the time we get to checking the binary expressions. The original code worked by virtue of overfitting to *just* integer literals.

Not being able to detect when expressions are dependent inside template instantiations has been a pain for warnings since forever.

I believe that evaluating the expression would make this warning too broad and would need more testing that what was included here. Only handling UnaryOperator with IntegerLiteral sub-expression makes more sense for the warning, and adding in any new cases if we find them.

I agree that the warning is too broad right now and that's unintentional. However, manually handling every single case in the CFG as something special is fragile and what got us this bug in the first place. We have a constant expression evaluator (two, actually) and we shouldn't have to reimplement it a third time in the CFG. However, asking a GSoC mentee to address that is well beyond the scope of what they should be working on. So for now I'm going to revert this change, reopen the issue, and we'll discuss the next steps off-list with Usman.

This warning is now flagging some code which I believe is a false positive. In particular, when template arguments are involved, their values can be calculated during template instantiation, but they should be treated as variables instead of constants. For example:

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?