Missing tautological compare warnings due to unary operators

[Codesbyusman]

Summary

The patch mainly focuses on the no warnings for -Wtautological-compare. I t work fine for the positive numbers but doesn't for the negative numbers as mention in the post that this is because the warning explicitly checks for an IntegerLiteral AST node, but -1 is represented by a UnaryOperator with an IntegerLiteral sub-Expr.
So, for the solution lib/Analysis/CFG.cpp was updated by using the different hellper functions to identify that it is a integer lliteral and then will take value from the expression. The functions checkIncorrectEqualityOperator and also functions for the relational operators will be updated.

For the below code we have warnings:

if (0 == (5 | x)) {}

but not for

if (0 == (-5 | x)) {}

Thus updating to tackle this issue.

GitHub Issue

[Codesbyusman]

updating to more efficient

[Codesbyusman]

also updating for the Xor bitwise operator

[aaron.ballman]

Thanks for working on this! I have some suggestions, but you'll also need to add a release note at some point.

clang/lib/Analysis/CFG.cpp

971–978

Coding style fix.

982

Be sure to add test coverage for this change.

982–984

985–992

(Be sure to re-run clang format over the patch, in case I've messed up 80-col limits with my suggestions.)

991

It looks like you've added a spurious tab here.

993–994

Take this suggestion or leave it, as you want. I don't think the comment added much value because it's describing what the code does (which is reasonably obvious from reading the code given that the code is pretty simple) rather than describing why the code is necessary (which doesn't seem to be required in this case).

1001

This looks like an existing bug and suggests we're missing test coverage -- we create a TryResult object but do nothing with it; I suspect we wanted to return this result.

Can you try to add test coverage that hits this code path to verify the current behavior is wrong, then change it to return the result to make sure the behavior is corrected?

1003–1006

clang/test/SemaCXX/warn-unreachable.cpp

398–399

Why aren't we catching this one?

411–412

Or this one?

[Codesbyusman]

clang/lib/Analysis/CFG.cpp

971–978

ooh .. nice will update it

982

Yes working on that

1001

ok will try to make a test that will cover it

[Codesbyusman]

updating the code suggestions and test cases.

[aaron.ballman]

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

clang/test/Sema/warn-bitwise-compare.c

42

66

clang/test/SemaCXX/warn-unreachable.cpp

399–410

We should move these up above the TODO comment on line 399, since these are now getting the expected warnings.

[Codesbyusman]

In D130510#3682902, @aaron.ballman wrote:

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

where should I write this release note. Like in comment or somewhere else?

[aaron.ballman]

In D130510#3683078, @Codesbyusman wrote:

In D130510#3682902, @aaron.ballman wrote:

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

where should I write this release note. Like in comment or somewhere else?

All of Clang's release notes live in: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst and you'd want to put your note under Improvements to Clang's diagnostics (since you're making the diagnostic behavior better) or Bug Fixes (since there's a GitHub issue associated with it). I'd probably list it under Bug Fixes.

[Codesbyusman]

updated with the small fixes and addition of release note

[aaron.ballman]

LGTM! I'll make some editorial corrections for grammar to the release note when I land.

[Codesbyusman]

In D130510#3684740, @aaron.ballman wrote:

LGTM! I'll make some editorial corrections for grammar to the release note when I land.

Thank you.

[rtrieu]

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:

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;
}

void run(int x) {
    foo<4, int>(8);
}

In this instantiation, x is and'ed with the value 4 in different ways. sizeof(T) is type-dependent, I is value-dependent, and 4 is an integer literal. With your code, each of them would produce a warning. However, the first two values of 4 will change in different template instantiations, so the warning is not useful when it is okay for some instantiations to have constant values. 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.

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.

[aaron.ballman]

In D130510#3692654, @rtrieu wrote:

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:

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;
}

void run(int x) {
    foo<4, int>(8);
}

In this instantiation, x is and'ed with the value 4 in different ways. sizeof(T) is type-dependent, I is value-dependent, and 4 is an integer literal. With your code, each of them would produce a warning. However, the first two values of 4 will change in different template instantiations, so the warning is not useful when it is okay for some instantiations to have constant values.

Yeah, this is definitely a false positive we need to avoid, thank you for bringing it up!

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.

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.

[aaron.ballman]

I've reverted in c783ca0de1e1e00f364cf4745b8444a020ddd29b. Marking as requesting changes to make it clear more work needs to be done.

[rtrieu]

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.

I agree, the CFG should be as broadly applicable as possible, so using an evaluator there is fine. Manually handling every single case may be needed to keep the warning under control. It's possible to put that handling to the Sema side of things, right before the warning is emitted. There's already a filter for macros, so maybe adding the filtering logic there would be a good fix.

[Codesbyusman]

Adopted another approach working for the error caught. Kindly review this.

[rtrieu]

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

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;  // only warn here
}

void run(int x) {
    foo<4, int>(8);
}

clang/lib/Analysis/CFG.cpp

58

For debugging?

Have you tried llvm::errs() << "message"; ? A few streams are provided by LLVM support which most places have already, so no extra header is needed to make it work.

1016

Is this any better than just having the callers use EvaluateAsInt themselves?

[Codesbyusman]

In D130510#3698415, @rtrieu wrote:

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

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;  // only warn here
}

void run(int x) {
    foo<4, int>(8);
}

Yes sure, will update you

clang/lib/Analysis/CFG.cpp

58

For debugging?

Yes. I just forget to remove them. Will updaye them.

Have you tried llvm::errs() << "message"; ? A few streams are provided by LLVM support which most places have already, so no extra header is needed to make it work.

okay, Great. Will use in future. Thank you.

[rtrieu]

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.

[Codesbyusman]

updating for the missing tautological compare warnings.

[Codesbyusman]

updates

[Codesbyusman]

updated also the test cases.

[Codesbyusman]

updated.

[aaron.ballman]

This is looking much closer to what I think we had in mind, so I mostly have some cleanup suggestions.

clang/lib/Analysis/CFG.cpp

979

You can drop the IgnoreParens() here -- BoolExpr is either LHSExpr or RHSExpr, and both of those were initialized from a call to IgnoreParens().

1013–1015

1017–1019

Removing an unnecessary variable.

1020–1021

Coding style nit -- if the type is spelled out explicitly in the initialization, we tend to prefer using auto for the type so we don't have to repeat the type information twice.

Also removes a declaration that's not needed.

1027

This ensures everything past here doesn't have to care about parens.

1028–1031

1032

1033–1034

1041–1045

1047

If there's a unary expression operating on an integer literal... it has to be one of the listed operations above, otherwise I don't think you can get here. So might as well assert and bail out rather than return a value that we didn't properly perform the unary operation on.

1050–1058

[Codesbyusman]

updated.

[aaron.ballman]

Nice! Just a few more small nits to fix that I can see.

clang/lib/Analysis/CFG.cpp

1016–1021

Oops, I realized we could simplify this further and keep UnOp scoped more tightly to where it's used.

1026–1032

Another simplification to scope things more tightly.

1045–1047

[Codesbyusman]

updated.

[aaron.ballman]

This looks correct to me! Any further concerns @erichkeane or @rtrieu?

[rtrieu]

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.

Besides that, there is added support for multiple unary operators, but only minus is tested. Each one should have at least a positive and a negative test to show it is supported.

[aaron.ballman]

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.

Besides that, there is added support for multiple unary operators, but only minus is tested. Each one should have at least a positive and a negative test to show it is supported.

Good catch! I agree those tests should be added.

[Codesbyusman]

working on the test. Will update the patch soon.

[Codesbyusman]

updated test cases.

[rtrieu]

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.

In that case, I suggest adding a comment to pointing to the other function. I expected that some day, someone will notice that the calculation for literals is slightly different between the two warnings and we should be helpful to them. I have no other concerns.

[aaron.ballman]

In D130510#3729864, @rtrieu wrote:

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.

In that case, I suggest adding a comment to pointing to the other function. I expected that some day, someone will notice that the calculation for literals is slightly different between the two warnings and we should be helpful to them. I have no other concerns.

That's a good suggestion, I've added two suggested edits for the comment. Thanks for the extra set of eyes on this review @rtrieu!

clang/lib/Analysis/CFG.cpp

74–75

1015–1016

[Codesbyusman]

updated.

[ebevhan]

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

The failing case is:

void f(int a)
{
    (0 != (a | !0LL));
}

built with clang -cc1 -emit-llvm-bc -O2 -v -o foo.bc -x c foo.c

clang/lib/Analysis/CFG.cpp

1040

This isn't returning an APInt of the right width. It will construct an APInt with a width of the input value, but that isn't the same as the required width of what a logical not produces; that should have a width of 'int'.

[aaron.ballman]

In D130510#3817148, @ebevhan wrote:

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

Thank you for the feedback! I've addressed the issue in 96a79cb308d1b8c00a83b180d9fecc5d54bacb9c.

clang/lib/Analysis/CFG.cpp

1040

Good catch, that is correct -- logical not always returns an int.

[uabelho]

In D130510#3821641, @aaron.ballman wrote:

In D130510#3817148, @ebevhan wrote:

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

Thank you for the feedback! I've addressed the issue in 96a79cb308d1b8c00a83b180d9fecc5d54bacb9c.

Thanks!

clang/docs/ReleaseNotes.rst

 - Fix `#56772 <https://github.com/llvm/llvm-project/issues/56772>`_ - invalid
   destructor names were incorrectly accepted on template classes.
 - Improve compile-times with large dynamic array allocations with trivial
   constructors. This fixes
   `Issue 56774 <https://github.com/llvm/llvm-project/issues/56774>`_.
 - No longer assert/miscompile when trying to make a vectorized ``_BitInt`` type
   using the ``ext_vector_type`` attribute (the ``vector_size`` attribute was
   already properly diagnosing this case).
+- ``-Wtautological-compare`` missed warnings for tautological comparisons
+  involving a negative integer literal. This fixes
+  `Issue 42918 <https://github.com/llvm/llvm-project/issues/42918>`_.
   
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
   the enumeration's values. Fixes
   `Issue 50055: <https://github.com/llvm/llvm-project/issues/50055>`_.
 
 Non-comprehensive list of changes in this release

clang/lib/Analysis/CFG.cpp

 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DOTGraphTraits.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/GraphWriter.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <memory>
 #include <string>
 #include <tuple>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 
 static SourceLocation GetEndLoc(Decl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     if (Expr *Ex = VD->getInit())
       return Ex->getSourceRange().getEnd();
   return D->getLocation();
 }
 
 /// Returns true on constant values based around a single IntegerLiteral.
 /// Allow for use of parentheses, integer casts, and negative signs.
 static bool IsIntegerLiteralConstantExpr(const Expr *E) {
   // Allow parentheses
   E = E->IgnoreParens();
 
   // Allow conversions to different integer kind.
   if (const auto *CE = dyn_cast<CastExpr>(E)) {
     if (CE->getCastKind() != CK_IntegralCast)
       return false;
     E = CE->getSubExpr();
private:
   /// Find an incorrect equality comparison. Either with an expression
   /// evaluating to a boolean and a constant other than 0 and 1.
   /// e.g. if (!x == 10) or a bitwise and/or operation that always evaluates to
   /// true/false e.q. (x & 8) == 4.
   TryResult checkIncorrectEqualityOperator(const BinaryOperator *B) {
     const Expr *LHSExpr = B->getLHS()->IgnoreParens();
     const Expr *RHSExpr = B->getRHS()->IgnoreParens();
 
-    const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(LHSExpr);
+    Optional<llvm::APInt> IntLiteral1 =
+        getIntegerLiteralSubexpressionValue(LHSExpr);
     const Expr *BoolExpr = RHSExpr;
 
-    if (!IntLiteral) {
-      IntLiteral = dyn_cast<IntegerLiteral>(RHSExpr);
+    if (!IntLiteral1) {
+      IntLiteral1 = getIntegerLiteralSubexpressionValue(RHSExpr);
       BoolExpr = LHSExpr;
     }
 
-    if (!IntLiteral)
+    if (!IntLiteral1)
       return TryResult();
 
-    const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr);
+    const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr->IgnoreParens());
     if (BitOp && (BitOp->getOpcode() == BO_And ||
                   BitOp->getOpcode() == BO_Or)) {
       const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
       const Expr *RHSExpr2 = BitOp->getRHS()->IgnoreParens();
 
-      const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
+      Optional<llvm::APInt> IntLiteral2 =
+          getIntegerLiteralSubexpressionValue(LHSExpr2);
 
       if (!IntLiteral2)
-        IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
+        IntLiteral2 = getIntegerLiteralSubexpressionValue(RHSExpr2);
 
       if (!IntLiteral2)
         return TryResult();
 
-      llvm::APInt L1 = IntLiteral->getValue();
-      llvm::APInt L2 = IntLiteral2->getValue();
-      if ((BitOp->getOpcode() == BO_And && (L2 & L1) != L1) ||
-          (BitOp->getOpcode() == BO_Or  && (L2 | L1) != L1)) {
+      if ((BitOp->getOpcode() == BO_And &&
+           (*IntLiteral2 & *IntLiteral1) != *IntLiteral1) ||
+          (BitOp->getOpcode() == BO_Or &&
+           (*IntLiteral2 | *IntLiteral1) != *IntLiteral1)) {
         if (BuildOpts.Observer)
           BuildOpts.Observer->compareBitwiseEquality(B,
                                                      B->getOpcode() != BO_EQ);
         TryResult(B->getOpcode() != BO_EQ);
       }
     } else if (BoolExpr->isKnownToHaveBooleanValue()) {
-      llvm::APInt IntValue = IntLiteral->getValue();
-      if ((IntValue == 1) || (IntValue == 0)) {
+      if ((*IntLiteral1 == 1) || (*IntLiteral1 == 0)) {
         return TryResult();
       }
       return TryResult(B->getOpcode() != BO_EQ);
     }
 
     return TryResult();
   }
 
+  Optional<llvm::APInt> getIntegerLiteralSubexpressionValue(const Expr *E) {
+    // The value to return;
+    Optional<llvm::APInt> Value = llvm::None;
+
+    const IntegerLiteral *IntLiteral = nullptr;
+    const UnaryOperator *UnOp = dyn_cast<UnaryOperator>(E->IgnoreParens());
+
+    // If unary.
+    if (UnOp) {
+      // Get the sub expression of the unary expression and get the Integer
+      // Literal.
+      const Expr *SubExpr = UnOp->getSubExpr();
+      IntLiteral = dyn_cast<IntegerLiteral>(SubExpr->IgnoreParens());
+
+      if (IntLiteral) {
+
+        Value = IntLiteral->getValue();
+        // Getting the Operator Code to perform them manually.
+        const UnaryOperatorKind OpCode = UnOp->getOpcode();
+
+        // Perform the operation manually.
+        switch (OpCode) {
+        case UO_Plus:
+          return Value;
+        case UO_Minus:
+          return -*Value;
+        case UO_Not:
+          return ~*Value;
+        case UO_LNot:
+          return *Value = !*Value;
+        default:
+          return Value;
+        }
+      }
+
+    } else {
+
+      IntLiteral = dyn_cast<IntegerLiteral>(E->IgnoreParens());
+      if (IntLiteral)
+        return Value = IntLiteral->getValue();
+    }
+
+    return Value;
+  }
+
   TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
                                           const llvm::APSInt &Value1,
                                           const llvm::APSInt &Value2) {
     assert(Value1.isSigned() == Value2.isSigned());
     switch (Relation) {
       default:
         return TryResult();
       case BO_EQ:
         return TryResult(Value1 == Value2);

clang/test/Sema/warn-bitwise-compare.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
+#define mydefine2 -2
 
 enum {
   ZERO,
   ONE,
 };
 
 void f(int x) {
   if ((8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((-8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x & -8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((2 & x) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((x & -8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((-2 & x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+ 
   if ((x | 4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x | 3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+    
+  if ((x | -4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x | -3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((-5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x & 0xFFEB) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((0xFFDD | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
   if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if (!!((-8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+
   int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+  y = ((-8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+  y = ((3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
+  y = ((-3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
 
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
   if ((x | 4) != 4) {}
 
+  if ((-2 & x) != 4) {}
+  if ((x & -8) == -8) {}
+  if ((x & -8) != -8) {}
+  if ((x | -4) == -4) {}
+  if ((x | -4) != -4) {}
+
   if ((x & 9) == 8) {}
   if ((x & 9) != 8) {}
   if ((x | 4) == 5) {}
   if ((x | 4) != 5) {}
 
+  if ((x & -9) == -10) {}
+  if ((x & -9) != -10) {}
+  if ((x | -4) == -3) {}
+  if ((x | -4) != -3) {}
+
+  if ((x^0) == 0) {}
+
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
+
+  if ((x & mydefine2) == 8) {}
+  if ((x | mydefine2) == 4) {}
 }
 
 void g(int x) {
   if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
 
   if (x | -1) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (x | ONE) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
 
   if (x | ZERO) {}
 }

clang/test/SemaCXX/warn-unreachable.cpp

void tautological_compare(bool x, int y) {
 
   if (y < 10 || y > 5)  // expected-note {{silence}}
     calledFun();
   else
     calledFun();        // expected-warning {{will never be executed}}
 
   if (y == -1 && y != -1)  // expected-note {{silence}}
     calledFun();        // expected-warning {{will never be executed}}
 
-  // TODO: Extend warning to the following code:
-  if (x < -1)
-    calledFun();
-  if (x == -1)
-    calledFun();
+  if (x == -1)   // expected-note {{silence}}
+    calledFun(); // expected-warning {{will never be executed}}
 
-  if (x != -1)
+  if (x != -1)   // expected-note {{silence}}
     calledFun();
   else
+    calledFun(); // expected-warning {{will never be executed}}
+    
+  // TODO: Extend warning to the following code:
+  if (x < -1)
     calledFun();
+ 
   if (-1 > x)
     calledFun();
   else
     calledFun();
 
 }