C++ PATCH for c++/86190, bogus -Wsign-conversion warning

[Jason Merrill]

OK.

On Fri, Jul 13, 2018 at 11:43 PM, Marek Polacek <polacek@redhat.com> wrote:
> Ping.
>
> On Tue, Jul 03, 2018 at 09:35:24AM -0400, Marek Polacek wrote:
>> This PR complains about bogus -Wsign-conversion warning even with an
>> explicit static_cast.  It started with this hunk from the delayed folding
>> merge:
>>
>> @@ -5028,20 +5022,12 @@ cp_build_binary_op (location_t location,
>>
>>        if (short_compare)
>>     {
>> -     /* Don't write &op0, etc., because that would prevent op0
>> -        from being kept in a register.
>> -        Instead, make copies of the our local variables and
>> -        pass the copies by reference, then copy them back afterward.  */
>> -     tree xop0 = op0, xop1 = op1, xresult_type = result_type;
>> +     /* We call shorten_compare only for diagnostic-reason.  */
>> +     tree xop0 = fold_simple (op0), xop1 = fold_simple (op1),
>> +          xresult_type = result_type;
>>       enum tree_code xresultcode = resultcode;
>> -     tree val
>> -       = shorten_compare (location, &xop0, &xop1, &xresult_type,
>> +     shorten_compare (location, &xop0, &xop1, &xresult_type,
>>                    &xresultcode);
>> -     if (val != 0)
>> -       return cp_convert (boolean_type_node, val, complain);
>> -     op0 = xop0, op1 = xop1;
>> -     converted = 1;
>> -     resultcode = xresultcode;
>>     }
>>
>>        if ((short_compare || code == MIN_EXPR || code == MAX_EXPR)
>>
>> which means that converted is now unset so we go to
>>
>>  5350   if (! converted)
>>  5351     {
>>  5352       if (TREE_TYPE (op0) != result_type)
>>  5353         op0 = cp_convert_and_check (result_type, op0, complain);
>>  5354       if (TREE_TYPE (op1) != result_type)
>>  5355         op1 = cp_convert_and_check (result_type, op1, complain);
>>
>> and cp_convert_and_check gives those warning.  The direct comparison
>> of types instead of same_type_p means we can try to convert same types,
>> but it still wouldn't fix this PR.  What we should probably do is to
>> simply disable -Wsign-conversion conversion for comparison, because
>> -Wsign-compare will warn for those.  With this patch, the C++ FE will
>> follow what the C FE and clang++ do.
>>
>> Also fix some formatting that's been bothering me, while at it.
>>
>> Bootstrapped/regtested on x86_64-linux, ok for trunk/8?
>>
>> 2018-07-03  Marek Polacek  <polacek@redhat.com>
>>
>>       PR c++/86190 - bogus -Wsign-conversion warning
>>       * typeck.c (cp_build_binary_op): Fix formatting.  Add a warning
>>       sentinel.
>>
>>       * g++.dg/warn/Wsign-conversion-3.C: New test.
>>       * g++.dg/warn/Wsign-conversion-4.C: New test.
>>
>> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
>> index 3a4f1cdf479..cfd1dd8b150 100644
>> --- gcc/cp/typeck.c
>> +++ gcc/cp/typeck.c
>> @@ -5311,12 +5311,13 @@ cp_build_binary_op (location_t location,
>>
>>        if (short_compare)
>>       {
>> -       /* We call shorten_compare only for diagnostic-reason.  */
>> -       tree xop0 = fold_simple (op0), xop1 = fold_simple (op1),
>> -            xresult_type = result_type;
>> +       /* We call shorten_compare only for diagnostics.  */
>> +       tree xop0 = fold_simple (op0);
>> +       tree xop1 = fold_simple (op1);
>> +       tree xresult_type = result_type;
>>         enum tree_code xresultcode = resultcode;
>>         shorten_compare (location, &xop0, &xop1, &xresult_type,
>> -                            &xresultcode);
>> +                        &xresultcode);
>>       }
>>
>>        if ((short_compare || code == MIN_EXPR || code == MAX_EXPR)
>> @@ -5349,6 +5350,7 @@ cp_build_binary_op (location_t location,
>>       otherwise, it will be given type RESULT_TYPE.  */
>>    if (! converted)
>>      {
>> +      warning_sentinel w (warn_sign_conversion, short_compare);
>>        if (TREE_TYPE (op0) != result_type)
>>       op0 = cp_convert_and_check (result_type, op0, complain);
>>        if (TREE_TYPE (op1) != result_type)
>> diff --git gcc/testsuite/g++.dg/warn/Wsign-conversion-3.C gcc/testsuite/g++.dg/warn/Wsign-conversion-3.C
>> index e69de29bb2d..2c3fef31475 100644
>> --- gcc/testsuite/g++.dg/warn/Wsign-conversion-3.C
>> +++ gcc/testsuite/g++.dg/warn/Wsign-conversion-3.C
>> @@ -0,0 +1,13 @@
>> +// PR c++/86190
>> +// { dg-options "-Wsign-conversion -Wsign-compare" }
>> +
>> +typedef unsigned long sz_t;
>> +sz_t s();
>> +bool f(int i) { return s() < (unsigned long) i; }
>> +bool f2(int i) { return s() < static_cast<unsigned long>(i); }
>> +bool f3(int i) { return s() < i; } // { dg-warning "comparison of integer expressions of different signedness" }
>> +bool f4(int i) { return s() < (long) i; } // { dg-warning "comparison of integer expressions of different signedness" }
>> +bool f5(short int i) { return s() < (int) i; } // { dg-warning "comparison of integer expressions of different signedness" }
>> +bool f6(signed char i) { return s() < (int) i; } // { dg-warning "comparison of integer expressions of different signedness" }
>> +bool f7(unsigned char i) { return s() < i; }
>> +bool f8(signed char i) { return s() < i; } // { dg-warning "comparison of integer expressions of different signedness" }
>> diff --git gcc/testsuite/g++.dg/warn/Wsign-conversion-4.C gcc/testsuite/g++.dg/warn/Wsign-conversion-4.C
>> index e69de29bb2d..40814b95587 100644
>> --- gcc/testsuite/g++.dg/warn/Wsign-conversion-4.C
>> +++ gcc/testsuite/g++.dg/warn/Wsign-conversion-4.C
>> @@ -0,0 +1,14 @@
>> +// PR c++/86190
>> +// { dg-options "-Wsign-conversion -Wsign-compare" }
>> +
>> +typedef unsigned long size_t;
>> +
>> +struct vector {
>> +  typedef size_t size_type;
>> +  size_type size();
>> +};
>> +
>> +bool func(vector vec, int var)
>> +{
>> +  return vec.size() < static_cast<size_t>(var); // { dg-bogus "may change" }
>> +}
>
>         Marek