* [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings)
@ 2016-03-31 21:10 Patrick Palka
2016-03-31 22:22 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Patrick Palka @ 2016-03-31 21:10 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, Patrick Palka
-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
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 ();
+}
--
2.8.0.rc3.27.gade0865
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings)
2016-03-31 21:10 [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings) Patrick Palka
@ 2016-03-31 22:22 ` Jakub Jelinek
2016-03-31 22:23 ` Patrick Palka
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2016-03-31 22:22 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, jason
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings)
2016-03-31 22:22 ` Jakub Jelinek
@ 2016-03-31 22:23 ` Patrick Palka
2016-04-06 14:31 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Patrick Palka @ 2016-03-31 22:23 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Patrick Palka, gcc-patches, jason
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-06 14:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-31 21:10 [PATCH 2/2] Fix C++ side of PR c/70436 (missing -Wparentheses warnings) Patrick Palka
2016-03-31 22:22 ` Jakub Jelinek
2016-03-31 22:23 ` Patrick Palka
2016-04-06 14:31 ` Jason Merrill
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).