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
