public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Patrick Palka <patrick@parcs.ath.cx>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Patrick Palka <patrick@parcs.ath.cx>,
	gcc-patches@gcc.gnu.org,     jason@redhat.com
Subject: Re: [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings)
Date: Thu, 31 Mar 2016 22:23:00 -0000	[thread overview]
Message-ID: <alpine.DEB.2.20.11.1603311751190.1308@idea> (raw)
In-Reply-To: <20160331213759.GQ3017@tucnak.redhat.com>

On Thu, 31 Mar 2016, Jakub Jelinek wrote:

> 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
> 

Thanks, patch updated with your requested changes.  Here's an
incremental diff of the changes (since they are relatively minor):

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index af2a8f2..6dd6280 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 *if_p, bool);
+  (cp_parser *, bool *, 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 *, bool *if_p, const token_indent_info &);
+  (cp_parser *, bool *, const token_indent_info &);
 
 /* Declarations [gram.dcl.dcl] */
 
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-29.C b/gcc/testsuite/g++.dg/warn/Wparentheses-29.C
index 125e6b4..7832415 100644
--- a/gcc/testsuite/g++.dg/warn/Wparentheses-29.C
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-29.C
@@ -1,13 +1,15 @@
 /* PR c/70436  */
 /* { dg-options "-Wparentheses" }  */
 
-int a, b;
+int a, b, c;
 void bar (void);
 void baz (void);
 
 void
 foo (void)
 {
+  int i, j;
+
   if (a) /* { dg-warning "ambiguous" }  */
     for (;;)
       if (b)
@@ -31,6 +33,42 @@ foo (void)
           baz ();
 
   if (a) /* { dg-warning "ambiguous" }  */
+    while (1)
+      while (1)
+        if (b)
+          bar ();
+  else
+    baz ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    for (i = 0; i < 10; i++)
+      for (j = 0; j < 10; j++)
+        if (b)
+          bar ();
+  else
+    baz ();
+
+  if (a)
+    for (i = 0; i < 10; i++)
+      if (b) /* { dg-warning "ambiguous" }  */
+        for (j = 0; j < 10; j++)
+          if (c)
+            bar ();
+      else
+        baz ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
+    for (i = 0; i < 10; i++)
+      if (b)
+        for (j = 0; j < 10; j++)
+          if (c)
+            bar ();
+          else
+            baz ();
+  else
+    bar ();
+
+  if (a) /* { dg-warning "ambiguous" }  */
     for (;;)
       if (b)
         while (1)

  reply	other threads:[~2016-03-31 21:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-31 21:10 Patrick Palka
2016-03-31 22:22 ` Jakub Jelinek
2016-03-31 22:23   ` Patrick Palka [this message]
2016-04-06 14:31     ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.20.11.1603311751190.1308@idea \
    --to=patrick@parcs.ath.cx \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=jason@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).