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 <[email protected]>
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
