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