Re: c-family PATCH to improve -Wsign-compare (PR c/81417)

[Eric Gallager <egall at gwmail dot gwu dot edu>]

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.


>
> OK for trunk (with my "diagnostic messages" maintainer hat on).
>
> Thanks
> Dave
>
>> 2017-07-13  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..c903c080a33 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 (sop),
>> +		    TREE_TYPE (uop));
>>      }
>>
>>    /* 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..e53f87aa9a3 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: 'int' and 'unsigned int'" } */
>> +}
>> +
>> +int
>> +fn3 (signed long int a, unsigned long int b)
>> +{
>> +  return b < a; /* { dg-warning "comparison between signed and
>> unsigned integer expressions: 'long int' and 'long unsigned int'" }
>> */
>> +}
>> +
>> +int
>> +fn4 (signed short int a, unsigned int b)
>> +{
>> +  return b < a; /* { dg-warning "comparison between signed and
>> unsigned integer expressions: 'short int' and 'unsigned int'" } */
>> +}
>>
>> 	Marek
>