C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)
Martin Sebor
msebor@gmail.com
Mon Jul 31 17:07:00 GMT 2017
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
More information about the Gcc-patches
mailing list