On Tue, Jul 25, 2017 at 11:03:12AM -0400, David Malcolm wrote: > Thanks for updating the patch. > > There's still an issue with ordering in the updated patch. > > There are three orderings: > > (a) the order of the expressions in the source code (LHS CMP RHS) > > (b) the order of kinds of signedness in the messages (currently > hardcoded as "signed and unsigned", which doesn't respect (a)) > > (c) the order of the the types that are reported (currently done as > orig_op0 vs orig_op1, which if I'm reading the code is LHS vs RHS). > > So, as written (a) and (c) have the same order, but (b)'s order is > hardcoded, and so there could be a mismatch. > > All of the examples in the testcase are of the form > signed LHS with unsigned RHS. > > What happens if the LHS is unsigned, and the RHS is signed? e.g. > > int > fn5 (unsigned int a, signed int b) > { > return a < b; > } > > Presumably this case still merits a warning, but as written, the > warning would read: > > warning "comparison between signed and unsigned integer expressions: > 'unsigned int' and 'signed int' > > This seems rather awkward to me; in a less trivial example, I can imagine the > user having to take a moment to figure out which side of the expression has > which signedness. > > I think that any time we're reporting on the types of two sides of an > expression like this, we should follow the ordering in the user's code, i.e. > (a) above. The patch has (c) doing this, but the text (b) is problematic. > > I can see two ways of fixing this: > > (i) rework the text of the message so that it changes based on which side has > which signedness, e.g.: > > "comparison between signed and unsigned integer expressions" > vs > "comparison between unsigned and signed integer expressions" > > or, > > (ii) change the text of the message to not have an ordering. Clang has > "comparison of integers of different signs" - though I think this should say > "signedness", not "signs"; surely an instance of an int has a sign (e.g. "-3" > is negative), but a integer *type* has a signedness (e.g. "unsigned short"). > So I'd change it to say: > "comparison of integer expressions of different signedness" > > Please also add a testcase that covers this case (e.g. fn5 above).
I went with (ii). I've also added the new test: Bootstrapped/regtested on x86_64-linux and ppc64le-linux, ok for trunk? 2017-07-27 Marek Polacek <pola...@redhat.com> PR c/81417 * c-warn.c (warn_for_sign_compare): Tweak the warning message. Print the types. * c-c++-common/Wsign-compare-1.c: New test. * g++.dg/warn/Wsign-compare-2.C: Update dg-warning. * g++.dg/warn/Wsign-compare-4.C: Likewise. * g++.dg/warn/Wsign-compare-6.C: Likewise. * g++.dg/warn/compare1.C: Likewise. * gcc.dg/compare1.c: Likewise. * gcc.dg/compare2.c: Likewise. * gcc.dg/compare4.c: Likewise. * gcc.dg/compare5.c: Likewise. * gcc.dg/pr35430.c: Likewise. * gcc.dg/pr60087.c: Likewise. diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c index a8b38c1b98d..505070e5586 100644 --- gcc/c-family/c-warn.c +++ gcc/c-family/c-warn.c @@ -1891,9 +1891,10 @@ warn_for_sign_compare (location_t location, c_common_signed_type (base_type))) /* OK */; else - warning_at (location, - OPT_Wsign_compare, - "comparison between signed and unsigned integer expressions"); + warning_at (location, OPT_Wsign_compare, + "comparison of integer expressions of different " + "signedness: %qT and %qT", TREE_TYPE (orig_op0), + TREE_TYPE (orig_op1)); } /* Warn if two unsigned values are being compared in a size larger diff --git gcc/testsuite/c-c++-common/Wsign-compare-1.c gcc/testsuite/c-c++-common/Wsign-compare-1.c index e69de29bb2d..b9b17a99280 100644 --- gcc/testsuite/c-c++-common/Wsign-compare-1.c +++ gcc/testsuite/c-c++-common/Wsign-compare-1.c @@ -0,0 +1,33 @@ +/* PR c/81417 */ +/* { dg-do compile } */ +/* { dg-options "-Wsign-compare" } */ + +int +fn1 (signed int a, unsigned int b) +{ + return a < b; /* { dg-warning "comparison of integer expressions of different signedness: 'int' and 'unsigned int'" } */ +} + +int +fn2 (signed int a, unsigned int b) +{ + return b < a; /* { dg-warning "comparison of integer expressions of different signedness: 'unsigned int' and 'int'" } */ +} + +int +fn3 (signed long int a, unsigned long int b) +{ + return b < a; /* { dg-warning "comparison of integer expressions of different signedness: 'long unsigned int' and 'long int'" } */ +} + +int +fn4 (signed short int a, unsigned int b) +{ + return b < a; /* { dg-warning "comparison of integer expressions of different signedness: 'unsigned int' and 'short int'" } */ +} + +int +fn5 (unsigned int a, signed int b) +{ + return a < b; /* { dg-warning "comparison of integer expressions of different signedness: 'unsigned int' and 'int'" } */ +} diff --git gcc/testsuite/g++.dg/warn/Wsign-compare-2.C gcc/testsuite/g++.dg/warn/Wsign-compare-2.C index 2b8360648fa..b66e8b46565 100644 --- gcc/testsuite/g++.dg/warn/Wsign-compare-2.C +++ gcc/testsuite/g++.dg/warn/Wsign-compare-2.C @@ -6,5 +6,5 @@ void foo (__complex__ int i) { i == 0u; - i == ~1u; // { dg-warning "comparison between signed and unsigned integer expressions" } + i == ~1u; // { dg-warning "comparison of integer expressions of different signedness" } } diff --git gcc/testsuite/g++.dg/warn/Wsign-compare-4.C gcc/testsuite/g++.dg/warn/Wsign-compare-4.C index b3eb8e2a7de..e92ab137e3b 100644 --- gcc/testsuite/g++.dg/warn/Wsign-compare-4.C +++ gcc/testsuite/g++.dg/warn/Wsign-compare-4.C @@ -3,10 +3,10 @@ int foo(unsigned int *a, int b) { - return (*a) <= b; // { dg-warning "comparison between signed and unsigned" } + return (*a) <= b; // { dg-warning "comparison of integer expressions of different signedness" } } int bar(unsigned int *a, int b) { - return *a <= b; // { dg-warning "comparison between signed and unsigned" } + return *a <= b; // { dg-warning "comparison of integer expressions of different signedness" } } diff --git gcc/testsuite/g++.dg/warn/Wsign-compare-6.C gcc/testsuite/g++.dg/warn/Wsign-compare-6.C index 1f8af664c7b..71b4ebf1613 100644 --- gcc/testsuite/g++.dg/warn/Wsign-compare-6.C +++ gcc/testsuite/g++.dg/warn/Wsign-compare-6.C @@ -8,7 +8,7 @@ int main() int i = 5; int const ic = 5; - i < 5u; // { dg-warning "5:comparison between signed and unsigned" } + i < 5u; // { dg-warning "5:comparison of integer expressions of different signedness" } ic < 5u; FIVE < 5u; } diff --git gcc/testsuite/g++.dg/warn/compare1.C gcc/testsuite/g++.dg/warn/compare1.C index e00592262c6..38f4dffd1da 100644 --- gcc/testsuite/g++.dg/warn/compare1.C +++ gcc/testsuite/g++.dg/warn/compare1.C @@ -6,5 +6,5 @@ int f(unsigned a, int b) { - return a < b; /* { dg-warning "signed and unsigned" } */ + return a < b; /* { dg-warning "comparison of integer expressions of different signedness" } */ } diff --git gcc/testsuite/gcc.dg/compare1.c gcc/testsuite/gcc.dg/compare1.c index 17ba3ee2098..7becfbdb17f 100644 --- gcc/testsuite/gcc.dg/compare1.c +++ gcc/testsuite/gcc.dg/compare1.c @@ -37,5 +37,5 @@ int h(enum mm2 x) int i(enum mm2 x) { - return x == (tf?DI2:-1); /* { dg-warning "signed and unsigned" "case 4" } */ + return x == (tf?DI2:-1); /* { dg-warning "different signedness" "case 4" } */ } diff --git gcc/testsuite/gcc.dg/compare2.c gcc/testsuite/gcc.dg/compare2.c index 0625eb9c3f6..c309f1d00eb 100644 --- gcc/testsuite/gcc.dg/compare2.c +++ gcc/testsuite/gcc.dg/compare2.c @@ -18,11 +18,11 @@ void f(int x, unsigned int y) /* ?: branches are signed constants. */ x > (tf?64:-1); /* { dg-bogus "signed and unsigned" "case 5" } */ - y > (tf?64:-1); /* { dg-warning "signed and unsigned" "case 6" } */ + y > (tf?64:-1); /* { dg-warning "different signedness" "case 6" } */ /* ?: branches are (recursively) signed constants. */ x > (tf?64:(tf?128:-1)); /* { dg-bogus "signed and unsigned" "case 7" } */ - y > (tf?64:(tf?128:-1)); /* { dg-warning "signed and unsigned" "case 8" } */ + y > (tf?64:(tf?128:-1)); /* { dg-warning "different signedness" "case 8" } */ /* Statement expression. */ x > ({tf; 64;}); /* { dg-bogus "signed and unsigned" "case 9" } */ @@ -34,11 +34,11 @@ void f(int x, unsigned int y) /* Statement expression with signed ?:. */ x > ({tf; tf?64:-1;}); /* { dg-bogus "signed and unsigned" "case 13" } */ - y > ({tf; tf?64:-1;}); /* { dg-warning "signed and unsigned" "case 14" } */ + y > ({tf; tf?64:-1;}); /* { dg-warning "different signedness" "case 14" } */ /* Statement expression with recursive signed ?:. */ x > ({tf; tf?64:(tf?128:-1);}); /* { dg-bogus "signed and unsigned" "case 15" } */ - y > ({tf; tf?64:(tf?128:-1);}); /* { dg-warning "signed and unsigned" "case 16" } */ + y > ({tf; tf?64:(tf?128:-1);}); /* { dg-warning "different signedness" "case 16" } */ /* ?: branches are constants. */ tf ? x : (tf?64:32); /* { dg-bogus "conditional expression" "case 17" } */ diff --git gcc/testsuite/gcc.dg/compare4.c gcc/testsuite/gcc.dg/compare4.c index e5dad4d1e45..299d488edf6 100644 --- gcc/testsuite/gcc.dg/compare4.c +++ gcc/testsuite/gcc.dg/compare4.c @@ -10,7 +10,7 @@ int foo(int x, int y, unsigned u) { /* A COMPOUND_EXPR is non-negative if the last element is known to be non-negative. */ - if (u < (bar(), -1)) /*{ dg-warning "9:signed and unsigned" "COMPOUND_EXPR" }*/ + if (u < (bar(), -1)) /*{ dg-warning "9:different signedness" "COMPOUND_EXPR" }*/ return x; if (u < (bar(), 10)) return x; @@ -34,7 +34,7 @@ int foo(int x, int y, unsigned u) /* A MODIFY_EXPR is non-negative if the new value is known to be non-negative. */ - if (u < (x = -1)) /* { dg-warning "9:signed and unsigned" "MODIFY_EXPR" } */ + if (u < (x = -1)) /* { dg-warning "9:different signedness" "MODIFY_EXPR" } */ return x; if (u < (x = 10)) return x; diff --git gcc/testsuite/gcc.dg/compare5.c gcc/testsuite/gcc.dg/compare5.c index f19c575efb7..3a502f12de9 100644 --- gcc/testsuite/gcc.dg/compare5.c +++ gcc/testsuite/gcc.dg/compare5.c @@ -10,7 +10,7 @@ int foo(int x, int y, unsigned u) { /* A *_DIV_EXPR is non-negative if both operands are. */ - if (u < ((x=-22)/33)) /* { dg-warning "signed and unsigned" "DIV_EXPR" } */ + if (u < ((x=-22)/33)) /* { dg-warning "different signedness" "DIV_EXPR" } */ return x; if (u < ((x=22)/33)) @@ -25,7 +25,7 @@ int foo(int x, int y, unsigned u) /* A *_MOD_EXPR is non-negative if the first operand is. */ - if (u < ((x=-22)%33)) /* { dg-warning "signed and unsigned" "MOD_EXPR" } */ + if (u < ((x=-22)%33)) /* { dg-warning "different signedness" "MOD_EXPR" } */ return x; if (u < ((x=22)%-33)) diff --git gcc/testsuite/gcc.dg/pr35430.c gcc/testsuite/gcc.dg/pr35430.c index ab5e4cac8a5..7365ccfcf94 100644 --- gcc/testsuite/gcc.dg/pr35430.c +++ gcc/testsuite/gcc.dg/pr35430.c @@ -6,5 +6,5 @@ void foo (__complex__ int i) { i == 0u; - i == ~1u; /* { dg-warning "comparison between signed and unsigned integer expressions" } */ + i == ~1u; /* { dg-warning "comparison of integer expressions of different signedness" } */ } diff --git gcc/testsuite/gcc.dg/pr60087.c gcc/testsuite/gcc.dg/pr60087.c index 9cdd5897354..c6cf7aa4607 100644 --- gcc/testsuite/gcc.dg/pr60087.c +++ gcc/testsuite/gcc.dg/pr60087.c @@ -10,5 +10,5 @@ foo (unsigned int ui, int i) b = 0 != ~uc; /* { dg-warning "9:promoted ~unsigned is always non-zero" } */ b = 2 != ~uc; /* { dg-warning "9:comparison of promoted ~unsigned with constant" } */ b = uc == ~uc; /* { dg-warning "10:comparison of promoted ~unsigned with unsigned" } */ - b = i == ui; /* { dg-warning "9:comparison between signed and unsigned integer expressions" } */ + b = i == ui; /* { dg-warning "9:comparison of integer expressions of different signedness" } */ } Marek