| Message ID | 20170714132721.GI2890@redhat.com |
|---|---|
| State | New |
| Headers | show |
Ping. On Fri, Jul 14, 2017 at 03:27:21PM +0200, Marek Polacek wrote: > On Thu, Jul 13, 2017 at 04:59:20PM -0400, David Malcolm wrote: > > On Thu, 2017-07-13 at 16:39 -0400, Eric Gallager wrote: > > > On 7/13/17, David Malcolm <dmalcolm@redhat.com> wrote: > > > > On Thu, 2017-07-13 at 18:33 +0200, Marek Polacek wrote: > > > > > A tiny patch for -Wsign-compare so that is also prints the types > > > > > when > > > > > reporting a warning. > > > > > > > > > > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux, > > > > > ok > > > > > for trunk? > > > > > > > > Looks like it always display the types in the order signed then > > > > unsigned, which matches the text of the diagnostic, but not > > > > necessarily > > > > the ordering within the expression, which might be confusing if > > > > someone's comparing e.g. > > > > > > > > unsigned_a < signed_b > > > > > > > > > > Good catch, I forgot about that case when opening the original bug > > > that Marek posted this patch for... > > > > > > > But we already hardcode the ordering within the text of the > > > > diagnostic, > > > > so that feels excessively nit-picky. > > > > > > I don't think it's being excessively nit-picky; I think it'd make > > > more > > > sense to match the ordering of the expression. That's what clang > > > does: > > > > > > $ cat Wsign_compare.c > > > /* { dg-do compile } */ > > > > > > int foo(signed int a, unsigned int b) > > > { > > > return (a < b); > > > } > > > > > > int bar(unsigned int c, signed int d) > > > { > > > return (c < d); > > > } > > > > > > $ /sw/opt/llvm-3.1/bin/clang -c -Wsign-compare Wsign_compare.c > > > Wsign_compare.c:5:12: warning: comparison of integers of different > > > signs: 'int' and 'unsigned int' [-Wsign-compare] > > > return (a < b); > > > ~ ^ ~ > > > Wsign_compare.c:10:12: warning: comparison of integers of different > > > signs: 'unsigned int' and 'int' [-Wsign-compare] > > > return (c < d); > > > ~ ^ ~ > > > 2 warnings generated. > > > > That's much nicer. > > > > > > > > > > > > > OK for trunk (with my "diagnostic messages" maintainer hat on). > > > > Marek: I take it back; can you update the patch accordingly, please? > > > > (Note to self: always doublecheck the linked PR for context). > > Sure, here goes: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2017-07-14 Marek Polacek <polacek@redhat.com> > > PR c/81417 > * c-warn.c (warn_for_sign_compare): Print the types. > > * c-c++-common/Wsign-compare-1.c: New test. > > diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c > index b9378c2dbe2..7ff11821453 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 between signed and unsigned integer " > + "expressions: %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..0e01453e7d8 100644 > --- gcc/testsuite/c-c++-common/Wsign-compare-1.c > +++ gcc/testsuite/c-c++-common/Wsign-compare-1.c > @@ -0,0 +1,27 @@ > +/* PR c/81417 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wsign-compare" } */ > + > +int > +fn1 (signed int a, unsigned int b) > +{ > + return a < b; /* { dg-warning "comparison between signed and unsigned integer expressions: 'int' and 'unsigned int'" } */ > +} > + > +int > +fn2 (signed int a, unsigned int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'int'" } */ > +} > + > +int > +fn3 (signed long int a, unsigned long int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'long unsigned int' and 'long int'" } */ > +} > + > +int > +fn4 (signed short int a, unsigned int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'short int'" } */ > +} Marek
How hard would it be to also suppress the warning for benign
comparisons like C++ manages to do? E.g., C warns on this
code even though there's no problem with it:
int foo (unsigned int b)
{
const int a = 1;
return (a < b);
}
Martin
On 07/25/2017 03:45 AM, Marek Polacek wrote:
> Ping.
>
> On Fri, Jul 14, 2017 at 03:27:21PM +0200, Marek Polacek wrote:
>> On Thu, Jul 13, 2017 at 04:59:20PM -0400, David Malcolm wrote:
>>> On Thu, 2017-07-13 at 16:39 -0400, Eric Gallager wrote:
>>>> On 7/13/17, David Malcolm <dmalcolm@redhat.com> wrote:
>>>>> On Thu, 2017-07-13 at 18:33 +0200, Marek Polacek wrote:
>>>>>> A tiny patch for -Wsign-compare so that is also prints the types
>>>>>> when
>>>>>> reporting a warning.
>>>>>>
>>>>>> Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux,
>>>>>> ok
>>>>>> for trunk?
>>>>>
>>>>> Looks like it always display the types in the order signed then
>>>>> unsigned, which matches the text of the diagnostic, but not
>>>>> necessarily
>>>>> the ordering within the expression, which might be confusing if
>>>>> someone's comparing e.g.
>>>>>
>>>>> unsigned_a < signed_b
>>>>>
>>>>
>>>> Good catch, I forgot about that case when opening the original bug
>>>> that Marek posted this patch for...
>>>>
>>>>> But we already hardcode the ordering within the text of the
>>>>> diagnostic,
>>>>> so that feels excessively nit-picky.
>>>>
>>>> I don't think it's being excessively nit-picky; I think it'd make
>>>> more
>>>> sense to match the ordering of the expression. That's what clang
>>>> does:
>>>>
>>>> $ cat Wsign_compare.c
>>>> /* { dg-do compile } */
>>>>
>>>> int foo(signed int a, unsigned int b)
>>>> {
>>>> return (a < b);
>>>> }
>>>>
>>>> int bar(unsigned int c, signed int d)
>>>> {
>>>> return (c < d);
>>>> }
>>>>
>>>> $ /sw/opt/llvm-3.1/bin/clang -c -Wsign-compare Wsign_compare.c
>>>> Wsign_compare.c:5:12: warning: comparison of integers of different
>>>> signs: 'int' and 'unsigned int' [-Wsign-compare]
>>>> return (a < b);
>>>> ~ ^ ~
>>>> Wsign_compare.c:10:12: warning: comparison of integers of different
>>>> signs: 'unsigned int' and 'int' [-Wsign-compare]
>>>> return (c < d);
>>>> ~ ^ ~
>>>> 2 warnings generated.
>>>
>>> That's much nicer.
>>>
>>>>
>>>>>
>>>>> OK for trunk (with my "diagnostic messages" maintainer hat on).
>>>
>>> Marek: I take it back; can you update the patch accordingly, please?
>>>
>>> (Note to self: always doublecheck the linked PR for context).
>>
>> Sure, here goes:
>>
>> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>>
>> 2017-07-14 Marek Polacek <polacek@redhat.com>
>>
>> PR c/81417
>> * c-warn.c (warn_for_sign_compare): Print the types.
>>
>> * c-c++-common/Wsign-compare-1.c: New test.
>>
>> diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
>> index b9378c2dbe2..7ff11821453 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 between signed and unsigned integer "
>> + "expressions: %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..0e01453e7d8 100644
>> --- gcc/testsuite/c-c++-common/Wsign-compare-1.c
>> +++ gcc/testsuite/c-c++-common/Wsign-compare-1.c
>> @@ -0,0 +1,27 @@
>> +/* PR c/81417 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-Wsign-compare" } */
>> +
>> +int
>> +fn1 (signed int a, unsigned int b)
>> +{
>> + return a < b; /* { dg-warning "comparison between signed and unsigned integer expressions: 'int' and 'unsigned int'" } */
>> +}
>> +
>> +int
>> +fn2 (signed int a, unsigned int b)
>> +{
>> + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'int'" } */
>> +}
>> +
>> +int
>> +fn3 (signed long int a, unsigned long int b)
>> +{
>> + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'long unsigned int' and 'long int'" } */
>> +}
>> +
>> +int
>> +fn4 (signed short int a, unsigned int b)
>> +{
>> + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'short int'" } */
>> +}
>
> Marek
>
On Fri, 2017-07-14 at 15:27 +0200, Marek Polacek wrote: > On Thu, Jul 13, 2017 at 04:59:20PM -0400, David Malcolm wrote: > > On Thu, 2017-07-13 at 16:39 -0400, Eric Gallager wrote: > > > On 7/13/17, David Malcolm <dmalcolm@redhat.com> wrote: > > > > On Thu, 2017-07-13 at 18:33 +0200, Marek Polacek wrote: > > > > > A tiny patch for -Wsign-compare so that is also prints the > > > > > types > > > > > when > > > > > reporting a warning. > > > > > > > > > > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat > > > > > -linux, > > > > > ok > > > > > for trunk? > > > > > > > > Looks like it always display the types in the order signed then > > > > unsigned, which matches the text of the diagnostic, but not > > > > necessarily > > > > the ordering within the expression, which might be confusing if > > > > someone's comparing e.g. > > > > > > > > unsigned_a < signed_b > > > > > > > > > > Good catch, I forgot about that case when opening the original > > > bug > > > that Marek posted this patch for... > > > > > > > But we already hardcode the ordering within the text of the > > > > diagnostic, > > > > so that feels excessively nit-picky. > > > > > > I don't think it's being excessively nit-picky; I think it'd make > > > more > > > sense to match the ordering of the expression. That's what clang > > > does: > > > > > > $ cat Wsign_compare.c > > > /* { dg-do compile } */ > > > > > > int foo(signed int a, unsigned int b) > > > { > > > return (a < b); > > > } > > > > > > int bar(unsigned int c, signed int d) > > > { > > > return (c < d); > > > } > > > > > > $ /sw/opt/llvm-3.1/bin/clang -c -Wsign-compare Wsign_compare.c > > > Wsign_compare.c:5:12: warning: comparison of integers of > > > different > > > signs: 'int' and 'unsigned int' [-Wsign-compare] > > > return (a < b); > > > ~ ^ ~ > > > Wsign_compare.c:10:12: warning: comparison of integers of > > > different > > > signs: 'unsigned int' and 'int' [-Wsign-compare] > > > return (c < d); > > > ~ ^ ~ > > > 2 warnings generated. > > > > That's much nicer. > > > > > > > > > > > > > OK for trunk (with my "diagnostic messages" maintainer hat on). > > > > Marek: I take it back; can you update the patch accordingly, > > please? > > > > (Note to self: always doublecheck the linked PR for context). > > Sure, here goes: 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). Dave > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2017-07-14 Marek Polacek <polacek@redhat.com> > > PR c/81417 > * c-warn.c (warn_for_sign_compare): Print the types. > > * c-c++-common/Wsign-compare-1.c: New test. > > diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c > index b9378c2dbe2..7ff11821453 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 between signed and unsigned integer > " > + "expressions: %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..0e01453e7d8 100644 > --- gcc/testsuite/c-c++-common/Wsign-compare-1.c > +++ gcc/testsuite/c-c++-common/Wsign-compare-1.c > @@ -0,0 +1,27 @@ > +/* PR c/81417 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wsign-compare" } */ > + > +int > +fn1 (signed int a, unsigned int b) > +{ > + return a < b; /* { dg-warning "comparison between signed and > unsigned integer expressions: 'int' and 'unsigned int'" } */ > +} > + > +int > +fn2 (signed int a, unsigned int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and > unsigned integer expressions: 'unsigned int' and 'int'" } */ > +} > + > +int > +fn3 (signed long int a, unsigned long int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and > unsigned integer expressions: 'long unsigned int' and 'long int'" } > */ > +} > + > +int > +fn4 (signed short int a, unsigned int b) > +{ > + return b < a; /* { dg-warning "comparison between signed and > unsigned integer expressions: 'unsigned int' and 'short int'" } */ > +} > > Marek
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c index b9378c2dbe2..7ff11821453 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 between signed and unsigned integer " + "expressions: %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..0e01453e7d8 100644 --- gcc/testsuite/c-c++-common/Wsign-compare-1.c +++ gcc/testsuite/c-c++-common/Wsign-compare-1.c @@ -0,0 +1,27 @@ +/* PR c/81417 */ +/* { dg-do compile } */ +/* { dg-options "-Wsign-compare" } */ + +int +fn1 (signed int a, unsigned int b) +{ + return a < b; /* { dg-warning "comparison between signed and unsigned integer expressions: 'int' and 'unsigned int'" } */ +} + +int +fn2 (signed int a, unsigned int b) +{ + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'int'" } */ +} + +int +fn3 (signed long int a, unsigned long int b) +{ + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'long unsigned int' and 'long int'" } */ +} + +int +fn4 (signed short int a, unsigned int b) +{ + return b < a; /* { dg-warning "comparison between signed and unsigned integer expressions: 'unsigned int' and 'short int'" } */ +}