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)
next prev parent 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).