c++: Fix bogus -Wparentheses warning [PR95344]

Message ID 20200527002541.546573-1-polacek@redhat.com
State New
Headers show
Series
  • c++: Fix bogus -Wparentheses warning [PR95344]
Related show

Commit Message

Kewen.Lin via Gcc-patches May 27, 2020, 12:25 a.m.
Since r267272, which added location wrappers, cp_fold loses
TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and
that results in a bogus -Wparentheses warning.

I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"
and cp_fold_maybe_rvalue folds away the location wrapper and so we do
2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);
in cp_fold and the flag is lost.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9?

	PR c++/95344
	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Set TREE_NO_WARNING.

	* c-c++-common/Wparentheses-2.c: New test.
---
 gcc/cp/cp-gimplify.c                        |  5 ++++-
 gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c


base-commit: 56f03cd12be26828788a27f6f3c250041a958e45
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA

Comments

Kewen.Lin via Gcc-patches May 28, 2020, 9:01 p.m. | #1
On 5/26/20 8:25 PM, Marek Polacek wrote:
> Since r267272, which added location wrappers, cp_fold loses

> TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and

> that results in a bogus -Wparentheses warning.

> 

> I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"

> and cp_fold_maybe_rvalue folds away the location wrapper and so we do

> 2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);

> in cp_fold and the flag is lost.

> 

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9?

> 

> 	PR c++/95344

> 	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Set TREE_NO_WARNING.

> 

> 	* c-c++-common/Wparentheses-2.c: New test.

> ---

>   gcc/cp/cp-gimplify.c                        |  5 ++++-

>   gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++

>   2 files changed, 22 insertions(+), 1 deletion(-)

>   create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c

> 

> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c

> index 53d715dcd89..8b505dd878c 100644

> --- a/gcc/cp/cp-gimplify.c

> +++ b/gcc/cp/cp-gimplify.c

> @@ -2745,7 +2745,10 @@ cp_fold (tree x)

>   	    x = org_x;

>   	}

>         if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)

> -	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

> +	{

> +	  TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

> +	  TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);

> +	}


I wonder if we want to copy these flags lower down for any EXPR_P (x) 
where TREE_CODE (x) == code?

Jason
Kewen.Lin via Gcc-patches May 28, 2020, 11:11 p.m. | #2
On Thu, May 28, 2020 at 05:01:51PM -0400, Jason Merrill wrote:
> On 5/26/20 8:25 PM, Marek Polacek wrote:

> > Since r267272, which added location wrappers, cp_fold loses

> > TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and

> > that results in a bogus -Wparentheses warning.

> > 

> > I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"

> > and cp_fold_maybe_rvalue folds away the location wrapper and so we do

> > 2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);

> > in cp_fold and the flag is lost.

> > 

> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9?

> > 

> > 	PR c++/95344

> > 	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Set TREE_NO_WARNING.

> > 

> > 	* c-c++-common/Wparentheses-2.c: New test.

> > ---

> >   gcc/cp/cp-gimplify.c                        |  5 ++++-

> >   gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++

> >   2 files changed, 22 insertions(+), 1 deletion(-)

> >   create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c

> > 

> > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c

> > index 53d715dcd89..8b505dd878c 100644

> > --- a/gcc/cp/cp-gimplify.c

> > +++ b/gcc/cp/cp-gimplify.c

> > @@ -2745,7 +2745,10 @@ cp_fold (tree x)

> >   	    x = org_x;

> >   	}

> >         if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)

> > -	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

> > +	{

> > +	  TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

> > +	  TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);

> > +	}

> 

> I wonder if we want to copy these flags lower down for any EXPR_P (x) where

> TREE_CODE (x) == code?


Sounds good; I don't think we want to lose those flags when folding in general,
not just for MODIFY_EXPR.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --
Since r267272, which added location wrappers, cp_fold loses
TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and
that results in a bogus -Wparentheses warning.

I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"
and cp_fold_maybe_rvalue folds away the location wrapper and so we do
2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);
in cp_fold and the flag is lost.

	PR c++/95344
	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Don't set
	TREE_THIS_VOLATILE here.
	(cp_fold): Set it here along with TREE_NO_WARNING.

	* c-c++-common/Wparentheses-2.c: New test.
---
 gcc/cp/cp-gimplify.c                        |  8 ++++++--
 gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 53d715dcd89..d6723e44ec4 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2744,8 +2744,6 @@ cp_fold (tree x)
 	  else
 	    x = org_x;
 	}
-      if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)
-	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);
 
       break;
 
@@ -2994,6 +2992,12 @@ cp_fold (tree x)
       return org_x;
     }
 
+  if (EXPR_P (x) && TREE_CODE (x) == code)
+    {
+      TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);
+      TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);
+    }
+
   if (!c.evaluation_restricted_p ())
     {
       fold_cache->put (org_x, x);
diff --git a/gcc/testsuite/c-c++-common/Wparentheses-2.c b/gcc/testsuite/c-c++-common/Wparentheses-2.c
new file mode 100644
index 00000000000..1aa5d314ae7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wparentheses-2.c
@@ -0,0 +1,18 @@
+// PR c++/95344 - bogus -Wparentheses warning.
+// { dg-do compile }
+// { dg-options "-Wparentheses" }
+
+#ifndef __cplusplus
+# define bool _Bool
+# define true 1
+# define false 0
+#endif
+
+void
+f (int i)
+{
+  bool b = false;
+  if (i == 99 ? (b = true) : false) // { dg-bogus "suggest parentheses" }
+    {
+    }
+}

base-commit: 3d8d5ddb539a5254c7ef83414377f4c74c7701d4
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Kewen.Lin via Gcc-patches May 29, 2020, 4:30 p.m. | #3
On 5/28/20 7:11 PM, Marek Polacek wrote:
> On Thu, May 28, 2020 at 05:01:51PM -0400, Jason Merrill wrote:

>> On 5/26/20 8:25 PM, Marek Polacek wrote:

>>> Since r267272, which added location wrappers, cp_fold loses

>>> TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and

>>> that results in a bogus -Wparentheses warning.

>>>

>>> I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"

>>> and cp_fold_maybe_rvalue folds away the location wrapper and so we do

>>> 2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);

>>> in cp_fold and the flag is lost.

>>>

>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9?

>>>

>>> 	PR c++/95344

>>> 	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Set TREE_NO_WARNING.

>>>

>>> 	* c-c++-common/Wparentheses-2.c: New test.

>>> ---

>>>    gcc/cp/cp-gimplify.c                        |  5 ++++-

>>>    gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++

>>>    2 files changed, 22 insertions(+), 1 deletion(-)

>>>    create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c

>>>

>>> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c

>>> index 53d715dcd89..8b505dd878c 100644

>>> --- a/gcc/cp/cp-gimplify.c

>>> +++ b/gcc/cp/cp-gimplify.c

>>> @@ -2745,7 +2745,10 @@ cp_fold (tree x)

>>>    	    x = org_x;

>>>    	}

>>>          if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)

>>> -	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

>>> +	{

>>> +	  TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

>>> +	  TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);

>>> +	}

>>

>> I wonder if we want to copy these flags lower down for any EXPR_P (x) where

>> TREE_CODE (x) == code?

> 

> Sounds good; I don't think we want to lose those flags when folding in general,

> not just for MODIFY_EXPR.

> 

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?


OK.

> -- >8 --

> Since r267272, which added location wrappers, cp_fold loses

> TREE_NO_WARNING on a MODIFY_EXPR that finish_parenthesized_expr set, and

> that results in a bogus -Wparentheses warning.

> 

> I.e., previously we had "b = 1" but now we have "VIEW_CONVERT_EXPR<bool>(b) = 1"

> and cp_fold_maybe_rvalue folds away the location wrapper and so we do

> 2718             x = fold_build2_loc (loc, code, TREE_TYPE (x), op0, op1);

> in cp_fold and the flag is lost.

> 

> 	PR c++/95344

> 	* cp-gimplify.c (cp_fold) <case MODIFY_EXPR>: Don't set

> 	TREE_THIS_VOLATILE here.

> 	(cp_fold): Set it here along with TREE_NO_WARNING.

> 

> 	* c-c++-common/Wparentheses-2.c: New test.

> ---

>   gcc/cp/cp-gimplify.c                        |  8 ++++++--

>   gcc/testsuite/c-c++-common/Wparentheses-2.c | 18 ++++++++++++++++++

>   2 files changed, 24 insertions(+), 2 deletions(-)

>   create mode 100644 gcc/testsuite/c-c++-common/Wparentheses-2.c

> 

> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c

> index 53d715dcd89..d6723e44ec4 100644

> --- a/gcc/cp/cp-gimplify.c

> +++ b/gcc/cp/cp-gimplify.c

> @@ -2744,8 +2744,6 @@ cp_fold (tree x)

>   	  else

>   	    x = org_x;

>   	}

> -      if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)

> -	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

>   

>         break;

>   

> @@ -2994,6 +2992,12 @@ cp_fold (tree x)

>         return org_x;

>       }

>   

> +  if (EXPR_P (x) && TREE_CODE (x) == code)

> +    {

> +      TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);

> +      TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);

> +    }

> +

>     if (!c.evaluation_restricted_p ())

>       {

>         fold_cache->put (org_x, x);

> diff --git a/gcc/testsuite/c-c++-common/Wparentheses-2.c b/gcc/testsuite/c-c++-common/Wparentheses-2.c

> new file mode 100644

> index 00000000000..1aa5d314ae7

> --- /dev/null

> +++ b/gcc/testsuite/c-c++-common/Wparentheses-2.c

> @@ -0,0 +1,18 @@

> +// PR c++/95344 - bogus -Wparentheses warning.

> +// { dg-do compile }

> +// { dg-options "-Wparentheses" }

> +

> +#ifndef __cplusplus

> +# define bool _Bool

> +# define true 1

> +# define false 0

> +#endif

> +

> +void

> +f (int i)

> +{

> +  bool b = false;

> +  if (i == 99 ? (b = true) : false) // { dg-bogus "suggest parentheses" }

> +    {

> +    }

> +}

> 

> base-commit: 3d8d5ddb539a5254c7ef83414377f4c74c7701d4

>

Patch

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 53d715dcd89..8b505dd878c 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2745,7 +2745,10 @@  cp_fold (tree x)
 	    x = org_x;
 	}
       if (code == MODIFY_EXPR && TREE_CODE (x) == MODIFY_EXPR)
-	TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);
+	{
+	  TREE_THIS_VOLATILE (x) = TREE_THIS_VOLATILE (org_x);
+	  TREE_NO_WARNING (x) = TREE_NO_WARNING (org_x);
+	}
 
       break;
 
diff --git a/gcc/testsuite/c-c++-common/Wparentheses-2.c b/gcc/testsuite/c-c++-common/Wparentheses-2.c
new file mode 100644
index 00000000000..1aa5d314ae7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wparentheses-2.c
@@ -0,0 +1,18 @@ 
+// PR c++/95344 - bogus -Wparentheses warning.
+// { dg-do compile }
+// { dg-options "-Wparentheses" }
+
+#ifndef __cplusplus
+# define bool _Bool
+# define true 1
+# define false 0
+#endif
+
+void
+f (int i)
+{
+  bool b = false;
+  if (i == 99 ? (b = true) : false) // { dg-bogus "suggest parentheses" }
+    {
+    }
+}