[clang-tidy] Catch trivially true statements like a != 1 || a != 3

[watsond]

Summary

Catch trivially true statements of the form a != 1 || a != 3. Statements like
these are likely errors. They are particularly easy to miss when handling enums:

enum State {
RUNNING,
STOPPED,
STARTING,
ENDING
}

...
if (state != RUNNING || state != STARTING)
...

[etienneb]

Could you add some tests with enums (like the one in your description)?
This is missing and it's a nice to have.

clang-tidy/misc/RedundantExpressionCheck.cpp

244

The good news is that this code will be catch by this check!

if (OpcodeLHS == BO_NE || OpcodeLHS == BO_NE)   <<-- redundant expression

Should be:

if (OpcodeLHS == BO_NE || OpcodeRHS == BO_NE)

[etienneb]

clang-tidy/misc/RedundantExpressionCheck.cpp

244

btw, it should be:

if (OpcodeLHS == BO_NE && OpcodeRHS == BO_NE)

[watsond]

In D29858#675055, @etienneb wrote:

Could you add some tests with enums (like the one in your description)?
This is missing and it's a nice to have.

Added

clang-tidy/misc/RedundantExpressionCheck.cpp

244

Thanks for catching!

[watsond]

Fixed logic error, added enum test case

[alexfh]

LG unless Etienne has any concerns.

[etienneb]

thx for the improvement
lgtm

[alexfh]

Do you need someone to commit the patch for you?

[watsond]

In D29858#690700, @alexfh wrote:

Do you need someone to commit the patch for you?

I think so. This is my only LLVM patch, so I doubt I have permissions. Thanks in advance!

[watsond]

Following up. Was this checked in? Do I need to do anything further?

[alexfh]

In D29858#707897, @watsond wrote:

Following up. Was this checked in? Do I need to do anything further?

Committed the patch now. Thanks for the reminder!

clang-tidy/misc/RedundantExpressionCheck.cpp

if (OpcodeLHS == BO_LE && OpcodeRHS == BO_GE &&
       APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0)
     return true;
 
   // Handle cases where the constants are different: x > 4 || x <= 7.
   if ((OpcodeLHS == BO_GT || OpcodeLHS == BO_GE) &&
       (OpcodeRHS == BO_LT || OpcodeRHS == BO_LE))
     return true;
 
+  // Handle cases where constants are different but both ops are !=, like:
+  // x != 5 || x != 10
+  if (OpcodeLHS == BO_NE || OpcodeLHS == BO_NE)
+    return true;
+
   return false;
 }
 
 static bool rangeSubsumesRange(BinaryOperatorKind OpcodeLHS,
                                const APSInt &ValueLHS,
                                BinaryOperatorKind OpcodeRHS,
                                const APSInt &ValueRHS) {
   int Comparison = APSInt::compareValues(ValueLHS, ValueRHS);

test/clang-tidy/misc-redundant-expression.cpp

int TestRelational(int X, int Y) {
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
 
   if (X < 10 || X >= 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
   if (X <= 10 || X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
   if (X <= 10 || X >= 11) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (X != 7 || X != 14) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
 
   if (X < 7 && X < 6) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X < 7 && X < 7) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
   if (X < 7 && X < 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant