From 71d8d9b4688b09f78c50fc303100f45e389952b1 Mon Sep 17 00:00:00 2001 From: John McCall Date: Thu, 11 Mar 2010 19:43:18 +0000 Subject: [PATCH] Warn about comparing an unsigned expression with 0 in tautological ways. Patch by mikem! llvm-svn: 98279 --- .../clang/Basic/DiagnosticSemaKinds.td | 6 +++ clang/lib/Sema/Sema.h | 3 +- clang/lib/Sema/SemaChecking.cpp | 45 ++++++++++++++++--- clang/lib/Sema/SemaExpr.cpp | 5 +-- clang/lib/Sema/SemaExprCXX.cpp | 2 +- clang/test/Sema/compare.c | 8 ++++ 6 files changed, 57 insertions(+), 12 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 1e1c0aa31714..175fbe9c0f89 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -1857,6 +1857,12 @@ def warn_mixed_sign_comparison : Warning< def warn_mixed_sign_conditional : Warning< "operands of ? are integers of different signs: %0 and %1">, InGroup>, DefaultIgnore; +def warn_lunsigned_always_true_comparison : Warning< + "comparison of unsigned expression %0 is always %1">, + InGroup>, DefaultIgnore; +def warn_runsigned_always_true_comparison : Warning< + "comparison of %0 unsigned expression is always %1">, + InGroup>, DefaultIgnore; def err_invalid_this_use : Error< "invalid use of 'this' outside of a nonstatic member function">; diff --git a/clang/lib/Sema/Sema.h b/clang/lib/Sema/Sema.h index c9f39e2b992f..0012435124cb 100644 --- a/clang/lib/Sema/Sema.h +++ b/clang/lib/Sema/Sema.h @@ -4245,8 +4245,7 @@ private: SourceLocation ReturnLoc); void CheckFloatComparison(SourceLocation loc, Expr* lex, Expr* rex); void CheckSignCompare(Expr *LHS, Expr *RHS, SourceLocation Loc, - const PartialDiagnostic &PD, - bool Equality = false); + const BinaryOperator::Opcode* BinOpc = 0); void CheckImplicitConversion(Expr *E, QualType Target); }; diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 30a6ab465538..544d66b503e8 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -2011,10 +2011,9 @@ bool IsSameFloatAfterCast(const APValue &value, /// \param lex the left-hand expression /// \param rex the right-hand expression /// \param OpLoc the location of the joining operator -/// \param Equality whether this is an "equality-like" join, which -/// suppresses the warning in some cases +/// \param BinOpc binary opcode or 0 void Sema::CheckSignCompare(Expr *lex, Expr *rex, SourceLocation OpLoc, - const PartialDiagnostic &PD, bool Equality) { + const BinaryOperator::Opcode* BinOpc) { // Don't warn if we're in an unevaluated context. if (ExprEvalContexts.back().Context == Unevaluated) return; @@ -2075,17 +2074,51 @@ void Sema::CheckSignCompare(Expr *lex, Expr *rex, SourceLocation OpLoc, // If the signed operand is non-negative, then the signed->unsigned // conversion won't change it. - if (signedRange.NonNegative) + if (signedRange.NonNegative) { + // Emit warnings for comparisons of unsigned to integer constant 0. + // always false: x < 0 (or 0 > x) + // always true: x >= 0 (or 0 <= x) + llvm::APSInt X; + if (BinOpc && signedOperand->isIntegerConstantExpr(X, Context) && X == 0) { + if (signedOperand != lex) { + if (*BinOpc == BinaryOperator::LT) { + Diag(OpLoc, diag::warn_lunsigned_always_true_comparison) + << "< 0" << "false" + << lex->getSourceRange() << rex->getSourceRange(); + } + else if (*BinOpc == BinaryOperator::GE) { + Diag(OpLoc, diag::warn_lunsigned_always_true_comparison) + << ">= 0" << "true" + << lex->getSourceRange() << rex->getSourceRange(); + } + } + else { + if (*BinOpc == BinaryOperator::GT) { + Diag(OpLoc, diag::warn_runsigned_always_true_comparison) + << "0 >" << "false" + << lex->getSourceRange() << rex->getSourceRange(); + } + else if (*BinOpc == BinaryOperator::LE) { + Diag(OpLoc, diag::warn_runsigned_always_true_comparison) + << "0 <=" << "true" + << lex->getSourceRange() << rex->getSourceRange(); + } + } + } return; + } // For (in)equality comparisons, if the unsigned operand is a // constant which cannot collide with a overflowed signed operand, // then reinterpreting the signed operand as unsigned will not // change the result of the comparison. - if (Equality && unsignedRange.Width < unsignedWidth) + if (BinOpc && + (*BinOpc == BinaryOperator::EQ || *BinOpc == BinaryOperator::NE) && + unsignedRange.Width < unsignedWidth) return; - Diag(OpLoc, PD) + Diag(OpLoc, BinOpc ? diag::warn_mixed_sign_comparison + : diag::warn_mixed_sign_conditional) << lt << rt << lex->getSourceRange() << rex->getSourceRange(); } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 2249579ba4e1..5d352ceb0472 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -4105,7 +4105,7 @@ QualType Sema::CheckConditionalOperands(Expr *&Cond, Expr *&LHS, Expr *&RHS, if (getLangOptions().CPlusPlus) return CXXCheckConditionalOperands(Cond, LHS, RHS, QuestionLoc); - CheckSignCompare(LHS, RHS, QuestionLoc, diag::warn_mixed_sign_conditional); + CheckSignCompare(LHS, RHS, QuestionLoc); UsualUnaryConversions(Cond); UsualUnaryConversions(LHS); @@ -5282,8 +5282,7 @@ QualType Sema::CheckCompareOperands(Expr *&lex, Expr *&rex, SourceLocation Loc, if (lex->getType()->isVectorType() || rex->getType()->isVectorType()) return CheckVectorCompareOperands(lex, rex, Loc, isRelational); - CheckSignCompare(lex, rex, Loc, diag::warn_mixed_sign_comparison, - (Opc == BinaryOperator::EQ || Opc == BinaryOperator::NE)); + CheckSignCompare(lex, rex, Loc, &Opc); // C99 6.5.8p3 / C99 6.5.9p4 if (lex->getType()->isArithmeticType() && rex->getType()->isArithmeticType()) diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index b9c8afa195fd..e1e5efa7d3a9 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -2085,7 +2085,7 @@ QualType Sema::CXXCheckConditionalOperands(Expr *&Cond, Expr *&LHS, Expr *&RHS, if (LHS->isTypeDependent() || RHS->isTypeDependent()) return Context.DependentTy; - CheckSignCompare(LHS, RHS, QuestionLoc, diag::warn_mixed_sign_conditional); + CheckSignCompare(LHS, RHS, QuestionLoc); // C++0x 5.16p2 // If either the second or the third operand has type (cv) void, ... diff --git a/clang/test/Sema/compare.c b/clang/test/Sema/compare.c index 2821a935c3c7..7c8c36f0c145 100644 --- a/clang/test/Sema/compare.c +++ b/clang/test/Sema/compare.c @@ -274,3 +274,11 @@ void test4() { if (value < (unsigned long) &ptr4) // expected-warning {{comparison of integers of different signs}} return; } + +// PR4807 +int test5(unsigned int x) { + return (x < 0) // expected-warning {{comparison of unsigned expression < 0 is always false}} + && (0 > x) // expected-warning {{comparison of 0 > unsigned expression is always false}} + && (x >= 0) // expected-warning {{comparison of unsigned expression >= 0 is always true}} + && (0 <= x); // expected-warning {{comparison of 0 <= unsigned expression is always true}} +}