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

Message ID 20180703133524.GD5927@redhat.com
State New
Headers show
Series
  • C++ PATCH for c++/86190, bogus -Wsign-conversion warning
Related show

Commit Message

Marek Polacek July 3, 2018, 1:35 p.m.
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.

Comments

Marek Polacek July 13, 2018, 1:43 p.m. | #1
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
Jason Merrill July 18, 2018, 2:07 a.m. | #2
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

Patch

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" }
+}