From: John McCall Date: Thu, 5 Nov 2009 09:23:39 +0000 (+0000) Subject: Implement the conditional-operator part of -Wsign-compare. Turn X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b13c87f0c9705d91d5a3e134be9934c9ad531071;p=clang Implement the conditional-operator part of -Wsign-compare. Turn DiagnoseSignCompare into Sema::CheckSignCompare and call it from more places. Add some enumerator tests. These seem to expose some oddities in the types we're converting C++ enumerators to; in particular, they're converting to unsigned before int, which seems to contradict 4.5 [conv.prom] p2. Note to self: stop baiting Doug in my commit messages. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@86128 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index aae4d0e6a2..2309908df7 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1540,6 +1540,9 @@ def err_stmtexpr_file_scope : Error< def warn_mixed_sign_comparison : Warning< "comparison of integers of different signs: %0 and %1">, InGroup>; +def warn_mixed_sign_conditional : Warning< + "operands of ? are integers of different signs: %0 and %1">, + InGroup>; def err_invalid_this_use : Error< "invalid use of 'this' outside of a nonstatic member function">; diff --git a/lib/Sema/Sema.h b/lib/Sema/Sema.h index e97417680b..4c81cb18de 100644 --- a/lib/Sema/Sema.h +++ b/lib/Sema/Sema.h @@ -1609,6 +1609,9 @@ public: void DiagnoseSentinelCalls(NamedDecl *D, SourceLocation Loc, Expr **Args, unsigned NumArgs); + void CheckSignCompare(Expr *LHS, Expr *RHS, SourceLocation Loc, + const PartialDiagnostic &PD); + virtual ExpressionEvaluationContext PushExpressionEvaluationContext(ExpressionEvaluationContext NewContext); diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index c9a28a74ee..f1d6f2bb17 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -3340,6 +3340,8 @@ 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); + UsualUnaryConversions(Cond); UsualUnaryConversions(LHS); UsualUnaryConversions(RHS); @@ -4428,8 +4430,8 @@ QualType Sema::CheckShiftOperands(Expr *&lex, Expr *&rex, SourceLocation Loc, } /// Implements -Wsign-compare. -static void DiagnoseSignCompare(Sema &S, Expr *lex, Expr *rex, - BinaryOperator::Opcode Opc, SourceLocation OpLoc) { +void Sema::CheckSignCompare(Expr *lex, Expr *rex, SourceLocation OpLoc, + const PartialDiagnostic &PD) { QualType lt = lex->getType(), rt = rex->getType(); // Only warn if both operands are integral. @@ -4450,14 +4452,14 @@ static void DiagnoseSignCompare(Sema &S, Expr *lex, Expr *rex, // If the value is a non-negative integer constant, then the // signed->unsigned conversion won't change it. llvm::APSInt value; - if (signedOperand->isIntegerConstantExpr(value, S.Context)) { + if (signedOperand->isIntegerConstantExpr(value, Context)) { assert(value.isSigned() && "result of signed expression not signed"); if (value.isNonNegative()) return; } - S.Diag(OpLoc, diag::warn_mixed_sign_comparison) + Diag(OpLoc, PD) << lex->getType() << rex->getType() << lex->getSourceRange() << rex->getSourceRange(); } @@ -4470,7 +4472,7 @@ QualType Sema::CheckCompareOperands(Expr *&lex, Expr *&rex, SourceLocation Loc, if (lex->getType()->isVectorType() || rex->getType()->isVectorType()) return CheckVectorCompareOperands(lex, rex, Loc, isRelational); - DiagnoseSignCompare(*this, lex, rex, Opc, Loc); + CheckSignCompare(lex, rex, Loc, diag::warn_mixed_sign_comparison); // C99 6.5.8p3 / C99 6.5.9p4 if (lex->getType()->isArithmeticType() && rex->getType()->isArithmeticType()) diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index 6fdecc2675..dc57681573 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -1603,6 +1603,8 @@ 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); + // C++0x 5.16p2 // If either the second or the third operand has type (cv) void, ... QualType LTy = LHS->getType(); diff --git a/test/Sema/compare.c b/test/Sema/compare.c index 50b40e459c..9cbbfba935 100644 --- a/test/Sema/compare.c +++ b/test/Sema/compare.c @@ -13,6 +13,18 @@ int ints(long a, unsigned long b) { ((short)a == b) + // expected-warning {{comparison of integers of different signs}} (a == (unsigned int) b) + // expected-warning {{comparison of integers of different signs}} (a == (unsigned short) b); // expected-warning {{comparison of integers of different signs}} + + enum Enum {B}; + return (a == B) + + ((int)a == B) + + ((short)a == B) + + (a == (unsigned int) B) + // expected-warning {{comparison of integers of different signs}} + (a == (unsigned short) B); // expected-warning {{comparison of integers of different signs}} + + // Should be able to prove all of these are non-negative. + return (b == (long) B) + + (b == (int) B) + + (b == (short) B); } int equal(char *a, const char *b) { diff --git a/test/Sema/conditional-expr.c b/test/Sema/conditional-expr.c index 1f0a9deb5e..3bfeae5d4c 100644 --- a/test/Sema/conditional-expr.c +++ b/test/Sema/conditional-expr.c @@ -34,6 +34,25 @@ void foo() { typedef void *asdf; *(0 ? (asdf) 0 : &x) = 10; + + unsigned long test0 = 5; + test0 = test0 ? (long) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? (int) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? (short) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (long) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (int) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (short) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (long) 10; + test0 = test0 ? test0 : (int) 10; + test0 = test0 ? test0 : (short) 10; + test0 = test0 ? (long) 10 : test0; + test0 = test0 ? (int) 10 : test0; + test0 = test0 ? (short) 10 : test0; + + enum Enum { EVal }; + test0 = test0 ? EVal : test0; + test0 = test0 ? EVal : (int) test0; // okay: EVal is an int + test0 = test0 ? (unsigned) EVal : (int) test0; // expected-warning {{operands of ? are integers of different signs}} } int Postgresql() { diff --git a/test/SemaCXX/compare.cpp b/test/SemaCXX/compare.cpp new file mode 100644 index 0000000000..806b078e8d --- /dev/null +++ b/test/SemaCXX/compare.cpp @@ -0,0 +1,15 @@ +// RUN: clang-cc -fsyntax-only -pedantic -verify %s + +int test0(long a, unsigned long b) { + enum Enum {B}; + return (a == B) + // expected-warning {{comparison of integers of different signs}} + ((int)a == B) + // expected-warning {{comparison of integers of different signs}} + ((short)a == B) + // expected-warning {{comparison of integers of different signs}} + (a == (unsigned int) B) + // expected-warning {{comparison of integers of different signs}} + (a == (unsigned short) B); // expected-warning {{comparison of integers of different signs}} + + // Should be able to prove all of these are non-negative. + return (b == (long) B) + + (b == (int) B) + + (b == (short) B); +} diff --git a/test/SemaCXX/conditional-expr.cpp b/test/SemaCXX/conditional-expr.cpp index fea3324b5f..da2dd67d06 100644 --- a/test/SemaCXX/conditional-expr.cpp +++ b/test/SemaCXX/conditional-expr.cpp @@ -156,8 +156,8 @@ void test() i1 = i1 ? i1 : ir1; int *pi1 = i1 ? &i1 : 0; pi1 = i1 ? 0 : &i1; - i1 = i1 ? i1 : EVal; - i1 = i1 ? EVal : i1; + i1 = i1 ? i1 : EVal; // expected-warning {{operands of ? are integers of different signs}} ?? + i1 = i1 ? EVal : i1; // expected-warning {{operands of ? are integers of different signs}} ?? d1 = i1 ? 'c' : 4.0; d1 = i1 ? 4.0 : 'c'; Base *pb = i1 ? (Base*)0 : (Derived*)0; @@ -177,6 +177,24 @@ void test() (void)&(i1 ? flds.b1 : flds.i1); // expected-error {{address of bit-field requested}} (void)&(i1 ? flds.i1 : flds.b1); // expected-error {{address of bit-field requested}} + + unsigned long test0 = 5; + test0 = test0 ? (long) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? (int) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? (short) test0 : test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (long) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (int) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (short) test0; // expected-warning {{operands of ? are integers of different signs}} + test0 = test0 ? test0 : (long) 10; + test0 = test0 ? test0 : (int) 10; + test0 = test0 ? test0 : (short) 10; + test0 = test0 ? (long) 10 : test0; + test0 = test0 ? (int) 10 : test0; + test0 = test0 ? (short) 10 : test0; + + test0 = test0 ? EVal : test0; + test0 = test0 ? EVal : (int) test0; // expected-warning {{operands of ? are integers of different signs}} + // Note the thing that this does not test: since DR446, various situations // *must* create a separate temporary copy of class objects. This can only // be properly tested at runtime, though.