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 aae4d0e6a2c0acabfcd7625b867d6056b2bea122..2309908df7b4e580602d12d742938692aa41060d 100644 (file)
--- 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<DiagGroup<"sign-compare">>;
+def warn_mixed_sign_conditional : Warning<
+  "operands of ? are integers of different signs: %0 and %1">,
+  InGroup<DiagGroup<"sign-compare">>;
 
 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 e97417680b085f090ecd14dea97e13a5c0b30be9..4c81cb18de718bd5a8f51b79b0cb42c1736739f4 100644 (file)
--- 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 c9a28a74ee03eb40e55a35f8f351e451d01ec489..f1d6f2bb17ce4b8b18971a1fee77b5c35d128bf7 100644 (file)
--- 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 6fdecc26752137a412cbcc813462f22b7c3e7cf9..dc57681573568959eddfc27574a21b8800154554 100644 (file)
--- 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 50b40e459c39e783dbf7cffb684b1e5fb994a197..9cbbfba935bd048da83f7eafdb05ca4a09be2b30 100644 (file)
--- 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 1f0a9deb5e47d84cbef80a01bde85a252a006eb5..3bfeae5d4c5eb1fde738fdddc0bb87490f5b6cf4 100644 (file)
--- 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 (file)
index 0000000..806b078
--- /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 fea3324b5fd527e08230119d69b63b41974cc72a..da2dd67d061d85903d2cb50583c248a7a552f322 100644 (file)
--- 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.