This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thu, 2017-07-27 at 16:42 +0200, Marek Polacek wrote:
> 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?

[...]

Thanks for updating it.  LGTM.

Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]