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

[Marek Polacek <polacek at redhat dot com>]

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