| Message ID | 20170713163349.GE2890@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 But we already hardcode the ordering within the text of the diagnostic, so that feels excessively nit-picky. 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
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 >
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).
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'" } */ +}