Slightly modified patch in response to points brought up in Code Review.
On Fri, Jul 15, 2011 at 2:31 PM, Richard Trieu <[email protected]> wrote:
> Patch to completely remove warning for mixed signedness of conditional
> operands.
>
> Still available at http://codereview.appspot.com/4731042/
>
>
> On Thu, Jul 14, 2011 at 4:30 PM, Chandler Carruth <[email protected]>wrote:
>
>> On Thu, Jul 14, 2011 at 4:24 PM, Richard Trieu <[email protected]> wrote:
>>
>>> The main motivation is to make -Wsign-compare only about checking signs
>>> during comparisons. Since the operands of the conditional operator aren't
>>> compared, it seems out of place to check it there.
>>
>>
>> So, howabout a patch to just remove this part of -Wsign-compare? I mean,
>> if there is a good reason to keep it, then cool, I just don't think we need
>> another warning here...
>>
>
>
Index: test/Sema/conditional-expr.c
===================================================================
--- test/Sema/conditional-expr.c (revision 135185)
+++ test/Sema/conditional-expr.c (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -Wsign-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -Wsign-conversion %s
void foo() {
*(0 ? (double *)0 : (void *)0) = 0;
// FIXME: GCC doesn't consider the the following two statements to be errors.
@@ -36,12 +36,12 @@
*(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 ? (long) test0 : test0; // expected-warning {{operand of ? changes signedness: 'long' to 'unsigned long'}}
+ test0 = test0 ? (int) test0 : test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? (short) test0 : test0; // expected-warning {{operand of ? changes signedness: 'short' to 'unsigned long'}}
+ test0 = test0 ? test0 : (long) test0; // expected-warning {{operand of ? changes signedness: 'long' to 'unsigned long'}}
+ test0 = test0 ? test0 : (int) test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? test0 : (short) test0; // expected-warning {{operand of ? changes signedness: 'short' to 'unsigned long'}}
test0 = test0 ? test0 : (long) 10;
test0 = test0 ? test0 : (int) 10;
test0 = test0 ? test0 : (short) 10;
@@ -51,10 +51,13 @@
enum Enum { EVal };
test0 = test0 ? EVal : test0;
- test0 = test0 ? EVal : (int) test0; // okay: EVal is an int
- test0 = test0 ? // expected-warning {{operands of ? are integers of different signs}}
- (unsigned) EVal
- : (int) test0;
+ test0 = test0 ? EVal : (int) test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? test0 : EVal;
+ test0 = test0 ? (int) test0 : EVal; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+
+ int test1;
+ test1 = test0 ? EVal : (int) test0;
+ test1 = test0 ? (int) test0 : EVal;
}
int Postgresql() {
Index: test/SemaCXX/compare.cpp
===================================================================
--- test/SemaCXX/compare.cpp (revision 135185)
+++ test/SemaCXX/compare.cpp (working copy)
@@ -212,3 +212,14 @@
int pr7536() {
return (kMax > 0);
}
+
+// -Wsign-compare should not warn when ?: operands have different signedness.
+// This will be caught by -Wsign-conversion
+void test3() {
+ unsigned long a;
+ signed long b;
+ (void) (true ? a : b);
+ (void) (true ? (unsigned int)a : (signed int)b);
+ (void) (true ? b : a);
+ (void) (true ? (unsigned char)b : (signed char)a);
+}
Index: test/SemaCXX/warn-sign-conversion.cpp
===================================================================
--- test/SemaCXX/warn-sign-conversion.cpp (revision 135185)
+++ test/SemaCXX/warn-sign-conversion.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -Wsign-compare %s
+// RUN: %clang_cc1 -verify -fsyntax-only -Wsign-conversion %s
// NOTE: When a 'enumeral mismatch' warning is implemented then expect several
// of the following cases to be impacted.
@@ -25,15 +25,23 @@
int c1 = 1 ? i : Foo<bool>::C;
int c2 = 1 ? Foo<bool>::C : i;
- int d1 = 1 ? i : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d2 = 1 ? Foo<bool>::D : i; // expected-warning {{operands of ? are integers of different signs}}
- int d3 = 1 ? B : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d4 = 1 ? Foo<bool>::D : B; // expected-warning {{operands of ? are integers of different signs}}
+ int d1a = 1 ? i : Foo<bool>::D; // expected-warning {{test1::Foo<bool>::<anonymous enum at }}
+ int d1b = 1 ? i : Foo<bool>::D; // expected-warning {{warn-sign-conversion.cpp:13:5>' to 'int'}}
+ int d2a = 1 ? Foo<bool>::D : i; // expected-warning {{operand of ? changes signedness: 'test1::Foo<bool>::<anonymous enum at }}
+ int d2b = 1 ? Foo<bool>::D : i; // expected-warning {{SemaCXX/warn-sign-conversion.cpp:13:5>' to 'int'}}
+ int d3a = 1 ? B : Foo<bool>::D; // expected-warning {{operand of ? changes signedness: 'test1::Foo<bool>::<anonymous enum at }}
+ int d3b = 1 ? B : Foo<bool>::D; // expected-warning {{warn-sign-conversion.cpp:13:5>' to 'int'}}
+ int d4a = 1 ? Foo<bool>::D : B; // expected-warning {{operand of ? changes signedness: 'test1::Foo<bool>::<anonymous enum at }}
+ int d4b = 1 ? Foo<bool>::D : B; // expected-warning {{warn-sign-conversion.cpp:13:5>' to 'int'}}
- int e1 = 1 ? i : E; // expected-warning {{operands of ? are integers of different signs}}
- int e2 = 1 ? E : i; // expected-warning {{operands of ? are integers of different signs}}
- int e3 = 1 ? E : B; // expected-warning {{operands of ? are integers of different signs}}
- int e4 = 1 ? B : E; // expected-warning {{operands of ? are integers of different signs}}
+ int e1a = 1 ? i : E; // expected-warning {{operand of ? changes signedness: 'test1::<anonymous enum at }}
+ int e1b = 1 ? i : E; // expected-warning {{warn-sign-conversion.cpp:16:3>' to 'int'}}
+ int e2a = 1 ? E : i; // expected-warning {{operand of ? changes signedness: 'test1::<anonymous enum at }}
+ int e2b = 1 ? E : i; // expected-warning {{warn-sign-conversion.cpp:16:3>' to 'int'}}
+ int e3a = 1 ? E : B; // expected-warning {{operand of ? changes signedness: 'test1::<anonymous enum at }}
+ int e3b = 1 ? E : B; // expected-warning {{warn-sign-conversion.cpp:16:3>' to 'int'}}
+ int e4a = 1 ? B : E; // expected-warning {{operand of ? changes signedness: 'test1::<anonymous enum at }}
+ int e4b = 1 ? B : E; // expected-warning {{warn-sign-conversion.cpp:16:3>' to 'int'}}
}
}
@@ -59,14 +67,14 @@
int c1 = 1 ? i : Foo<bool>::C;
int c2 = 1 ? Foo<bool>::C : i;
- int d1 = 1 ? i : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d2 = 1 ? Foo<bool>::D : i; // expected-warning {{operands of ? are integers of different signs}}
- int d3 = 1 ? B : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d4 = 1 ? Foo<bool>::D : B; // expected-warning {{operands of ? are integers of different signs}}
+ int d1 = 1 ? i : Foo<bool>::D; // expected-warning {{operand of ? changes signedness: 'test2::Foo<bool>::Named4' to 'int'}}
+ int d2 = 1 ? Foo<bool>::D : i; // expected-warning {{operand of ? changes signedness: 'test2::Foo<bool>::Named4' to 'int'}}
+ int d3 = 1 ? B : Foo<bool>::D; // expected-warning {{operand of ? changes signedness: 'test2::Foo<bool>::Named4' to 'int'}}
+ int d4 = 1 ? Foo<bool>::D : B; // expected-warning {{operand of ? changes signedness: 'test2::Foo<bool>::Named4' to 'int'}}
- int e1 = 1 ? i : E; // expected-warning {{operands of ? are integers of different signs}}
- int e2 = 1 ? E : i; // expected-warning {{operands of ? are integers of different signs}}
- int e3 = 1 ? E : B; // expected-warning {{operands of ? are integers of different signs}}
- int e4 = 1 ? B : E; // expected-warning {{operands of ? are integers of different signs}}
+ int e1 = 1 ? i : E; // expected-warning {{operand of ? changes signedness: 'test2::Named5' to 'int'}}
+ int e2 = 1 ? E : i; // expected-warning {{operand of ? changes signedness: 'test2::Named5' to 'int'}}
+ int e3 = 1 ? E : B; // expected-warning {{operand of ? changes signedness: 'test2::Named5' to 'int'}}
+ int e4 = 1 ? B : E; // expected-warning {{operand of ? changes signedness: 'test2::Named5' to 'int'}}
}
}
Index: test/SemaCXX/conditional-expr.cpp
===================================================================
--- test/SemaCXX/conditional-expr.cpp (revision 135185)
+++ test/SemaCXX/conditional-expr.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++0x -Wsign-compare %s
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++0x -Wsign-conversion %s
// C++ rules for ?: are a lot stricter than C rules, and have to take into
// account more conversion options.
@@ -180,12 +180,12 @@
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 ? (long) test0 : test0; // expected-warning {{operand of ? changes signedness: 'long' to 'unsigned long'}}
+ test0 = test0 ? (int) test0 : test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? (short) test0 : test0; // expected-warning {{operand of ? changes signedness: 'short' to 'unsigned long'}}
+ test0 = test0 ? test0 : (long) test0; // expected-warning {{operand of ? changes signedness: 'long' to 'unsigned long'}}
+ test0 = test0 ? test0 : (int) test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? test0 : (short) test0; // expected-warning {{operand of ? changes signedness: 'short' to 'unsigned long'}}
test0 = test0 ? test0 : (long) 10;
test0 = test0 ? test0 : (int) 10;
test0 = test0 ? test0 : (short) 10;
@@ -194,8 +194,14 @@
test0 = test0 ? (short) 10 : test0;
test0 = test0 ? EVal : test0;
- test0 = test0 ? EVal : (int) test0;
+ test0 = test0 ? EVal : (int) test0; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ test0 = test0 ? test0 : EVal;
+ test0 = test0 ? (int) test0 : EVal; // expected-warning {{operand of ? changes signedness: 'int' to 'unsigned long'}}
+ int test1;
+ test1 = test0 ? EVal : (int) test0;
+ test1 = test0 ? (int) test0 : EVal;
+
// 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.
Index: test/SemaCXX/warn-sign-compare.cpp
===================================================================
--- test/SemaCXX/warn-sign-compare.cpp (revision 135185)
+++ test/SemaCXX/warn-sign-compare.cpp (working copy)
@@ -1,72 +0,0 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -Wsign-compare %s
-
-// NOTE: When a 'enumeral mismatch' warning is implemented then expect several
-// of the following cases to be impacted.
-
-// namespace for anonymous enums tests
-namespace test1 {
- enum { A };
- enum { B = -1 };
-
- template <typename T> struct Foo {
- enum { C };
- enum { D = ~0U };
- };
-
- enum { E = ~0U };
-
- void doit_anonymous( int i ) {
- int a1 = 1 ? i : A;
- int a2 = 1 ? A : i;
-
- int b1 = 1 ? i : B;
- int b2 = 1 ? B : i;
-
- int c1 = 1 ? i : Foo<bool>::C;
- int c2 = 1 ? Foo<bool>::C : i;
-
- int d1 = 1 ? i : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d2 = 1 ? Foo<bool>::D : i; // expected-warning {{operands of ? are integers of different signs}}
- int d3 = 1 ? B : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d4 = 1 ? Foo<bool>::D : B; // expected-warning {{operands of ? are integers of different signs}}
-
- int e1 = 1 ? i : E; // expected-warning {{operands of ? are integers of different signs}}
- int e2 = 1 ? E : i; // expected-warning {{operands of ? are integers of different signs}}
- int e3 = 1 ? E : B; // expected-warning {{operands of ? are integers of different signs}}
- int e4 = 1 ? B : E; // expected-warning {{operands of ? are integers of different signs}}
- }
-}
-
-// namespace for named enums tests
-namespace test2 {
- enum Named1 { A };
- enum Named2 { B = -1 };
-
- template <typename T> struct Foo {
- enum Named3 { C };
- enum Named4 { D = ~0U };
- };
-
- enum Named5 { E = ~0U };
-
- void doit_anonymous( int i ) {
- int a1 = 1 ? i : A;
- int a2 = 1 ? A : i;
-
- int b1 = 1 ? i : B;
- int b2 = 1 ? B : i;
-
- int c1 = 1 ? i : Foo<bool>::C;
- int c2 = 1 ? Foo<bool>::C : i;
-
- int d1 = 1 ? i : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d2 = 1 ? Foo<bool>::D : i; // expected-warning {{operands of ? are integers of different signs}}
- int d3 = 1 ? B : Foo<bool>::D; // expected-warning {{operands of ? are integers of different signs}}
- int d4 = 1 ? Foo<bool>::D : B; // expected-warning {{operands of ? are integers of different signs}}
-
- int e1 = 1 ? i : E; // expected-warning {{operands of ? are integers of different signs}}
- int e2 = 1 ? E : i; // expected-warning {{operands of ? are integers of different signs}}
- int e3 = 1 ? E : B; // expected-warning {{operands of ? are integers of different signs}}
- int e4 = 1 ? B : E; // expected-warning {{operands of ? are integers of different signs}}
- }
-}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 135185)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -3039,9 +3039,6 @@
def warn_mixed_sign_comparison : Warning<
"comparison of integers of different signs: %0 and %1">,
InGroup<SignCompare>, DefaultIgnore;
-def warn_mixed_sign_conditional : Warning<
- "operands of ? are integers of different signs: %0 and %1">,
- InGroup<SignCompare>, DefaultIgnore;
def warn_lunsigned_always_true_comparison : Warning<
"comparison of unsigned%select{| enum}2 expression %0 is always %1">,
InGroup<TautologicalCompare>;
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp (revision 135185)
+++ lib/Sema/SemaChecking.cpp (working copy)
@@ -3241,29 +3241,16 @@
CC))
return;
- // ...and -Wsign-compare isn't...
- if (!S.Diags.getDiagnosticLevel(diag::warn_mixed_sign_conditional, CC))
- return;
-
// ...then check whether it would have warned about either of the
// candidates for a signedness conversion to the condition type.
- if (E->getType() != T) {
- Suspicious = false;
- CheckImplicitConversion(S, E->getTrueExpr()->IgnoreParenImpCasts(),
+ if (E->getType() == T) return;
+
+ Suspicious = false;
+ CheckImplicitConversion(S, E->getTrueExpr()->IgnoreParenImpCasts(),
+ E->getType(), CC, &Suspicious);
+ if (!Suspicious)
+ CheckImplicitConversion(S, E->getFalseExpr()->IgnoreParenImpCasts(),
E->getType(), CC, &Suspicious);
- if (!Suspicious)
- CheckImplicitConversion(S, E->getFalseExpr()->IgnoreParenImpCasts(),
- E->getType(), CC, &Suspicious);
- if (!Suspicious)
- return;
- }
-
- // If so, emit a diagnostic under -Wsign-compare.
- Expr *lex = E->getTrueExpr()->IgnoreParenImpCasts();
- Expr *rex = E->getFalseExpr()->IgnoreParenImpCasts();
- S.Diag(E->getQuestionLoc(), diag::warn_mixed_sign_conditional)
- << lex->getType() << rex->getType()
- << lex->getSourceRange() << rex->getSourceRange();
}
/// AnalyzeImplicitConversions - Find and report any interesting
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
