On 07/31/2017 10:05 AM, Marek Polacek wrote:
On Mon, Jul 31, 2017 at 09:54:03AM -0600, Martin Sebor wrote:
On 07/31/2017 08:14 AM, Marek Polacek wrote:
This patch improves the diagnostic of -Wsign-compare for ?: by also printing
the types, similarly to my recent patch. But we can do even better here if we
actually point to the operand in question, so I passed the locations of the
operands from the parser. So instead of
x.c:8:16: warning: signed and unsigned type in conditional expression
[-Wsign-compare]
return x ? y : -1;
^
you'll now see:
x.c:8:18: warning: operand of conditional expression changes signedness: 'int'
to 'unsigned int' [-Wsign-compare]
return x ? y : -1;
^
I like that this is more informative than the last warning you
committed for this bug: it says what type the operand is converted
to. The last one only shows what the types of the operands are but
leaves users guessing as to what that might mean (integer promotion
rules are often poorly understood). Where the last warning prints
comparison of integer expressions of different signedness: ‘int’ and
‘unsigned int’
it would be nice to go back and add this detail to it as well, and
have it print something like this instead:
comparison of integer expressions of different signedness changes type of
the second operand from ‘int’ to ‘unsigned int’
Where constant expressions are involved it would also be helpful
to show the result of the conversion. For instance:
comparison between ‘int’ and ‘unsigned int’ changes the value of the
second operand from ‘-1’ to ‘4294967296’
Hmm, interesting. I could do that. How do other people feel about this?
Since you ask for input from others, let me mention that I also
would find it helpful if we could come up with some sort of high
level direction on what information to include in diagnostics and
how to present it (e.g., a section on the GCC Diagnostic Guidelines
page on the Wiki). Not only would it help guide us in implementing
new diagnostics but it would also result in a more consistent and
uniform user experience.
For what it's worth, my own preference is to err on the side of
providing more detail rather than less so the changes I made in
r248431 to -Wconversion reflect that. So for example there, GCC
might now print:
unsigned conversion from ‘int‘ to ‘unsigned char‘ changes value from
‘-128‘ to ‘128‘"
My suggestions above were based on the style I chose here.
Martin
