diff mbox

[2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings)

Message ID 1459457644-20663-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka March 31, 2016, 8:54 p.m. UTC 
-Wparentheses currently warns about an ambiguous "else" in this code

  if (a)
    if (b)
      bar ();
  else
    baz ();

but it fails to warn if there is an iteration statement between the
inner and outer ifs:

  if (a)
    for (;;)
      if (b)
        bar ();
  else
    baz ();

To fix this it's sufficient to just thread the IF_P parameter in
cp_parser_statement() through to cp_parser_iteration_statement() and
then to cp_parser_already_scoped_statement() and finally back to
cp_parser_statement().  In cp_parser_iteration_statement(), avoid
passing IF_P through a do-while statement, and in
cp_parser_already_scoped_statement() avoid passing IF_P through a
compound statement.

I think that covers all the vanilla C++ constructs that this warning has
to consider.  As for C++ extensions, we still fail to warn for

  if (a)
  #pragma GCC ivdep
     while (1)
       if (b)
         bar ();
  else
    baz ();

and

  if (a)
    _Cilk_for (int i = 0; i < 10; i++)
      if (b)
        bar ();
  else
    baz ();

and probably other constructs.  I suppose support for this can be
implemented in a subsequent patch if deemed appropriate at this stage.
It would probably just involve more threading of the parameter IF_P.

Is this OK to commit after bootstrap + regtesting?

gcc/cp/ChangeLog:

	PR c/70436
	* parser.c (cp_parser_iteration_statement): New parameter IF_P.
	Pass it through to cp_parser_already_scoped_statement.
	(cp_parser_already_scoped_statement): New parameter IF_P.  Pass
	it through to cp_parser_statement.
	(cp_parser_statement): Pass IF_P through to
	cp_parser_iteration_statement.
	(cp_parser_pragma): Adjust call to
	cp_parser_iteration_statement.

gcc/testsuite/ChangeLog:

	PR c/70436
	* g++.dg/warn/Wparentheses-29.C: New test.
---
 gcc/cp/parser.c                             | 20 +++---
 gcc/testsuite/g++.dg/warn/Wparentheses-29.C | 97 +++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-29.C

Comments

Jakub Jelinek March 31, 2016, 9:37 p.m. UTC | #1 
On Thu, Mar 31, 2016 at 04:54:04PM -0400, Patrick Palka wrote:
> I think that covers all the vanilla C++ constructs that this warning has
> to consider.  As for C++ extensions, we still fail to warn for
> 
>   if (a)
>   #pragma GCC ivdep
>      while (1)
>        if (b)
>          bar ();
>   else
>     baz ();
> 
> and
> 
>   if (a)
>     _Cilk_for (int i = 0; i < 10; i++)
>       if (b)
>         bar ();
>   else
>     baz ();
> 
> and probably other constructs.  I suppose support for this can be
> implemented in a subsequent patch if deemed appropriate at this stage.
> It would probably just involve more threading of the parameter IF_P.
> 
> Is this OK to commit after bootstrap + regtesting?

If this makes it in, I can take care of OpenMP, perhaps Cilk+ too.

> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -2104,7 +2104,7 @@ static tree cp_parser_selection_statement
>  static tree cp_parser_condition
>    (cp_parser *);
>  static tree cp_parser_iteration_statement
> -  (cp_parser *, bool);
> +  (cp_parser *, bool *if_p, bool);

I wouldn't add a named argument where all others are unnamed in the
prototype.

>  static bool cp_parser_for_init_statement
>    (cp_parser *, tree *decl);
>  static tree cp_parser_for
> @@ -2127,7 +2127,7 @@ static void cp_parser_declaration_statement
>  static tree cp_parser_implicitly_scoped_statement
>    (cp_parser *, bool *, const token_indent_info &, vec<tree> * = NULL);
>  static void cp_parser_already_scoped_statement
> -  (cp_parser *, const token_indent_info &);
> +  (cp_parser *, bool *if_p, const token_indent_info &);

Likewise here.

> +void
> +foo (void)
> +{
> +  if (a) /* { dg-warning "ambiguous" }  */
> +    for (;;)
> +      if (b)
> +        bar ();
> +      else
> +        baz ();

What about multiple nested for or while loops, like:
  if (a)
    for (i = 0; i < 10; i++)
      for (j = 0; j < 10; j++)
        if (b)
          bar ();
  else
    baz ();
and similarly for multiple nested while loops?
I only see while (1) for (;;) in the test.

Otherwise, I'll defer to Jason.

	Jakub
diff mbox

Patch

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 2f80856..af2a8f2 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -2104,7 +2104,7 @@  static tree cp_parser_selection_statement
 static tree cp_parser_condition
   (cp_parser *);
 static tree cp_parser_iteration_statement
-  (cp_parser *, bool);
+  (cp_parser *, bool *if_p, bool);
 static bool cp_parser_for_init_statement
   (cp_parser *, tree *decl);
 static tree cp_parser_for
@@ -2127,7 +2127,7 @@  static void cp_parser_declaration_statement
 static tree cp_parser_implicitly_scoped_statement
   (cp_parser *, bool *, const token_indent_info &, vec<tree> * = NULL);
 static void cp_parser_already_scoped_statement
-  (cp_parser *, const token_indent_info &);
+  (cp_parser *, bool *if_p, const token_indent_info &);
 
 /* Declarations [gram.dcl.dcl] */
 
@@ -10392,7 +10392,7 @@  cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 	case RID_WHILE:
 	case RID_DO:
 	case RID_FOR:
-	  statement = cp_parser_iteration_statement (parser, false);
+	  statement = cp_parser_iteration_statement (parser, if_p, false);
 	  break;
 
 	case RID_CILK_FOR:
@@ -10947,7 +10947,7 @@  cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 	    else
 	      {
 		/* This if statement does not have an else clause.  If
-		   NESTED_IF is true, then the then-clause is an if
+		   NESTED_IF is true, then the then-clause has an if
 		   statement which does have an else clause.  We warn
 		   about the potential ambiguity.  */
 		if (nested_if)
@@ -11544,7 +11544,7 @@  cp_parser_range_for_member_function (tree range, tree identifier)
    Returns the new WHILE_STMT, DO_STMT, FOR_STMT or RANGE_FOR_STMT.  */
 
 static tree
-cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
+cp_parser_iteration_statement (cp_parser* parser, bool *if_p, bool ivdep)
 {
   cp_token *token;
   enum rid keyword;
@@ -11582,7 +11582,7 @@  cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 	cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
 	/* Parse the dependent statement.  */
 	parser->in_statement = IN_ITERATION_STMT;
-	cp_parser_already_scoped_statement (parser, guard_tinfo);
+	cp_parser_already_scoped_statement (parser, if_p, guard_tinfo);
 	parser->in_statement = in_statement;
 	/* We're done with the while-statement.  */
 	finish_while_stmt (statement);
@@ -11627,7 +11627,7 @@  cp_parser_iteration_statement (cp_parser* parser, bool ivdep)
 
 	/* Parse the body of the for-statement.  */
 	parser->in_statement = IN_ITERATION_STMT;
-	cp_parser_already_scoped_statement (parser, guard_tinfo);
+	cp_parser_already_scoped_statement (parser, if_p, guard_tinfo);
 	parser->in_statement = in_statement;
 
 	/* We're done with the for-statement.  */
@@ -11937,7 +11937,7 @@  cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
    scope.  */
 
 static void
-cp_parser_already_scoped_statement (cp_parser* parser,
+cp_parser_already_scoped_statement (cp_parser* parser, bool *if_p,
 				    const token_indent_info &guard_tinfo)
 {
   /* If the token is a `{', then we must take special action.  */
@@ -11946,7 +11946,7 @@  cp_parser_already_scoped_statement (cp_parser* parser,
       token_indent_info body_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 
-      cp_parser_statement (parser, NULL_TREE, false, NULL);
+      cp_parser_statement (parser, NULL_TREE, false, if_p);
       token_indent_info next_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
       warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
@@ -37312,7 +37312,7 @@  cp_parser_pragma (cp_parser *parser, enum pragma_context context)
 	    cp_parser_error (parser, "for, while or do statement expected");
 	    return false;
 	  }
-	cp_parser_iteration_statement (parser, true);
+	cp_parser_iteration_statement (parser, NULL, true);
 	return true;
       }
 
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-29.C b/gcc/testsuite/g++.dg/warn/Wparentheses-29.C
new file mode 100644
index 0000000..125e6b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-29.C
@@ -0,0 +1,97 @@ 
+/* PR c/70436  */
+/* { dg-options "-Wparentheses" }  */
+
+int a, b;
+void bar (void);
+void baz (void);
+
+void
+foo (void)
+{
+  if (a) /* { dg-warning "ambiguous" }  */
+    for (;;)
+      if (b)
+        bar ();
+      else
+        baz ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    while (1)
+      if (b)
+        bar ();
+      else
+        baz ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    while (1)
+      for (;;)
+        if (b)
+          bar ();
+        else
+          baz ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    for (;;)
+      if (b)
+        while (1)
+          if (a)
+            bar ();
+          else
+            baz ();
+      else
+        bar ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    for (;;)
+      if (b)
+        while (1)
+          {
+            if (a) { bar (); } else { baz (); }
+          }
+      else
+        bar ();
+
+  if (a)
+    for (;;)
+      if (b)
+        bar ();
+      else
+        baz ();
+  else bar ();
+
+  if (a)
+    while (1)
+      if (b)
+        bar ();
+      else
+        baz ();
+  else bar ();
+
+  if (a)
+    for (;;)
+      {
+        if (b)
+          bar ();
+        else
+          baz ();
+      }
+
+  if (a)
+    {
+      for (;;)
+        if (b)
+          bar ();
+    }
+  else baz ();
+
+  if (a)
+    do
+      if (b) bar (); else baz ();
+    while (b);
+
+  if (a)
+    do
+      if (b) bar ();
+    while (b);
+  else baz ();
+}