Skip to content
Permalink
Browse files

Improve -Wtautological-overlap-compare

Allow this warning to detect a larger number of constant values, including
negative numbers, and handle non-int types better.

Differential Revision: https://reviews.llvm.org/D66044

llvm-svn: 372448
  • Loading branch information...
Richard Trieu
Richard Trieu committed Sep 21, 2019
1 parent 3e6590c commit 6541c7988b83b30b2590ca6b3e23585ae04a02a2
@@ -51,7 +51,8 @@ Major New Features
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

- ...
- -Wtautological-overlap-compare will warn on negative numbers and non-int
types.

Non-comprehensive list of changes in this release
-------------------------------------------------
@@ -70,11 +70,35 @@ static SourceLocation GetEndLoc(Decl *D) {
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();
}

// Allow negative numbers.
if (const auto *UO = dyn_cast<UnaryOperator>(E)) {
if (UO->getOpcode() != UO_Minus)
return false;
E = UO->getSubExpr();
}

return isa<IntegerLiteral>(E);
}

/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr.
/// constant expression or EnumConstantDecl from the given Expr. If it fails,
/// returns nullptr.
static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
E = E->IgnoreParens();
if (isa<IntegerLiteral>(E))
if (IsIntegerLiteralConstantExpr(E))
return E;
if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
@@ -121,11 +145,11 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) {
// User intent isn't clear if they're mixing int literals with enum
// constants.
if (isa<IntegerLiteral>(E1) != isa<IntegerLiteral>(E2))
if (isa<DeclRefExpr>(E1) != isa<DeclRefExpr>(E2))
return false;

// Integer literal comparisons, regardless of literal type, are acceptable.
if (isa<IntegerLiteral>(E1))
if (!isa<DeclRefExpr>(E1))
return true;

// IntegerLiterals are handled above and only EnumConstantDecls are expected
@@ -1081,6 +1105,10 @@ class CFGBuilder {
// * Variable x is equal to the largest literal.
// * Variable x is greater than largest literal.
bool AlwaysTrue = true, AlwaysFalse = true;
// Track value of both subexpressions. If either side is always
// true/false, another warning should have already been emitted.
bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
for (const llvm::APSInt &Value : Values) {
TryResult Res1, Res2;
Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
@@ -1096,10 +1124,16 @@ class CFGBuilder {
AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
}

LHSAlwaysTrue &= Res1.isTrue();
LHSAlwaysFalse &= Res1.isFalse();
RHSAlwaysTrue &= Res2.isTrue();
RHSAlwaysFalse &= Res2.isFalse();
}

if (AlwaysTrue || AlwaysFalse) {
if (BuildOpts.Observer)
if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
!RHSAlwaysFalse && BuildOpts.Observer)
BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
return TryResult(AlwaysTrue);
}
@@ -247,7 +247,7 @@ static bool isConfigurationValue(const Stmt *S,
}
case Stmt::UnaryOperatorClass: {
const UnaryOperator *UO = cast<UnaryOperator>(S);
if (UO->getOpcode() != UO_LNot)
if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus)
return false;
bool SilenceableCondValNotSet =
SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
@@ -547,6 +547,27 @@ int foo() {
}
} // namespace statement_expression_in_return

// CHECK-LABEL: int overlap_compare(int x)
// CHECK: [B2]
// CHECK-NEXT: 1: 1
// CHECK-NEXT: 2: return [B2.1];
// CHECK-NEXT: Preds (1): B3(Unreachable)
// CHECK-NEXT: Succs (1): B0
// CHECK: [B3]
// CHECK-NEXT: 1: x
// CHECK-NEXT: 2: [B3.1] (ImplicitCastExpr, LValueToRValue, int)
// CHECK-NEXT: 3: 5
// CHECK-NEXT: 4: [B3.2] > [B3.3]
// CHECK-NEXT: T: if [B4.5] && [B3.4]
// CHECK-NEXT: Preds (1): B4
// CHECK-NEXT: Succs (2): B2(Unreachable) B1
int overlap_compare(int x) {
if (x == -1 && x > 5)
return 1;

return 2;
}

// CHECK-LABEL: template<> int *PR18472<int>()
// CHECK: [B2 (ENTRY)]
// CHECK-NEXT: Succs (1): B1
@@ -141,3 +141,20 @@ int returns(int x) {
return x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
}

int integer_conversion(unsigned x, int y) {
return x > 4 || x < 10;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
return y > 4u || y < 10u;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
}

int negative_compare(int x) {
return x > -1 || x < 1;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
}

int no_warning(unsigned x) {
return x >= 0 || x == 1;
// no warning since "x >= 0" is caught by a different tautological warning.
}
@@ -433,7 +433,7 @@ void wrapOneInFixit(struct StructWithPointer *s) {
}

void unaryOpNoFixit() {
if (- 1)
if (~ 1)
return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
unaryOpNoFixit(); // expected-warning {{code will never be executed}}
}
@@ -393,6 +393,9 @@ void tautological_compare(bool x, int y) {
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();
@@ -408,6 +411,4 @@ void tautological_compare(bool x, int y) {
else
calledFun();

if (y == -1 && y != -1)
calledFun();
}

0 comments on commit 6541c79

Please sign in to comment.
You can’t perform that action at this time.