| Message ID | bc8380c1-1b62-04bc-5424-cddad7c33498@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2) | expand |
PS: maybe better using function_declarator_p??? I think I regression tested that variant too, at some point. Paolo.
On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> PS: maybe better using function_declarator_p?
I think so, yes. The relevant rule seems to be "The declarator shall
not specify a function or an array.", so let's check for arrays, too.
Jason
Hi, On 17/05/2018 16:58, Jason Merrill wrote: > On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini > <paolo.carlini@oracle.com> wrote: >> PS: maybe better using function_declarator_p? > I think so, yes. The relevant rule seems to be "The declarator shall > not specify a function or an array.", so let's check for arrays, too. Agreed. I had the amended patch ready when I noticed (again) that it wasn't addressing another related class of issues which involves declarators not followed by initializers. Thus I tried to fix those too, and the below which moves the check up appears to work fine, passes testing, etc. Are there any risks that an erroneous function / array as declarator is in fact a well formed expression?!? I haven't been able so far to construct examples... Thanks! Paolo. //////////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 260331) +++ cp/parser.c (working copy) @@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser, } } +/* Helper function for cp_parser_condition. Enforces [stmt.stmt]: + The declarator shall not specify a function or an array. Returns + TRUE is the declator is valid, FALSE otherwise. */ + +static bool +cp_parser_check_condition_declarator (cp_parser* parser, + cp_declarator *declarator, + location_t loc) +{ + if (function_declarator_p (declarator) + || declarator->kind == cdk_array) + { + if (declarator->kind == cdk_array) + error_at (loc, "an array type is not allowed here"); + else + error_at (loc, "a function type is not allowed here"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true, + /*or_comma=*/false, + /*consume_paren=*/false); + return false; + } + else + return true; +} + /* Parse a condition. condition: @@ -11571,6 +11598,7 @@ cp_parser_condition (cp_parser* parser) tree attributes; cp_declarator *declarator; tree initializer = NULL_TREE; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, @@ -11592,10 +11620,15 @@ cp_parser_condition (cp_parser* parser) if (cp_lexer_next_token_is_not (parser->lexer, CPP_EQ) && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) cp_parser_simulate_error (parser); - + /* If we did see an `=' or '{', then we are looking at a declaration for sure. */ - if (cp_parser_parse_definitely (parser)) + bool decl_p = cp_parser_parse_definitely (parser); + + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; + + if (decl_p) { tree pushed_scope; bool non_constant_p; Index: testsuite/g++.dg/cpp0x/cond1.C =================================================================== --- testsuite/g++.dg/cpp0x/cond1.C (nonexistent) +++ testsuite/g++.dg/cpp0x/cond1.C (working copy) @@ -0,0 +1,23 @@ +// PR c++/84588 +// { dg-do compile { target c++11 } } + +void foo() +{ + if (int bar() {}) // { dg-error "function type is not allowed" } + ; + + for (;int bar() {};) // { dg-error "function type is not allowed" } + ; + + while (int bar() {}) // { dg-error "function type is not allowed" } + ; + + if (int a[] {}) // { dg-error "array type is not allowed" } + ; + + for (;int a[] {};) // { dg-error "array type is not allowed" } + ; + + while (int a[] {}) // { dg-error "array type is not allowed" } + ; +} Index: testsuite/g++.dg/cpp1y/pr84588-1.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto) {}) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto) {};) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto) {}) // { dg-error "two or more data types|function type" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-2.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-2.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-2.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto);) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)) // { dg-error "two or more data types|function type" } + ; + }) {} +}; Index: testsuite/g++.dg/parse/cond6.C =================================================================== --- testsuite/g++.dg/parse/cond6.C (nonexistent) +++ testsuite/g++.dg/parse/cond6.C (working copy) @@ -0,0 +1,22 @@ +// PR c++/84588 + +void foo() +{ + if (int bar()) // { dg-error "function type is not allowed" } + ; + + for (;int bar();) // { dg-error "function type is not allowed" } + ; + + while (int bar()) // { dg-error "function type is not allowed" } + ; + + if (int a[]) // { dg-error "array type is not allowed" } + ; + + for (;int a[];) // { dg-error "array type is not allowed" } + ; + + while (int a[]) // { dg-error "array type is not allowed" } + ; +} Index: testsuite/g++.old-deja/g++.jason/cond.C =================================================================== --- testsuite/g++.old-deja/g++.jason/cond.C (revision 260331) +++ testsuite/g++.old-deja/g++.jason/cond.C (working copy) @@ -47,11 +47,10 @@ int main() if (struct B * foo = new B) ; - if (int f () = 1) // { dg-warning "extern" "extern" } - // { dg-error "is initialized like a variable" "var" { target *-*-* } .-1 } + if (int f () = 1) // { dg-error "function type" } ; - if (int a[2] = {1, 2}) // { dg-error "extended init" "" { target { ! c++11 } } } + if (int a[2] = {1, 2}) // { dg-error "array type" } ; }
On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > On 17/05/2018 16:58, Jason Merrill wrote: >> >> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >> <paolo.carlini@oracle.com> wrote: >>> >>> PS: maybe better using function_declarator_p? >> >> I think so, yes. The relevant rule seems to be "The declarator shall >> not specify a function or an array.", so let's check for arrays, too. > > Agreed. I had the amended patch ready when I noticed (again) that it wasn't > addressing another related class of issues which involves declarators not > followed by initializers. Thus I tried to fix those too, and the below which > moves the check up appears to work fine, passes testing, etc. Are there any > risks that an erroneous function / array as declarator is in fact a well > formed expression?!? That doesn't matter; if it parses as a declarator, it's a declarator, even if it's an ill-formed declarator. But... + bool decl_p = cp_parser_parse_definitely (parser); + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; ...if cp_parser_parse_definitely returns false, parsing as a declarator failed, so we shouldn't look at "declarator". Also, "here" in the diagnostic seems unnecessarily vague; we could be more specific. Maybe "condition declares a function/array"? Jason
Hi, On 18/05/2018 01:21, Jason Merrill wrote: > On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> On 17/05/2018 16:58, Jason Merrill wrote: >>> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >>> <paolo.carlini@oracle.com> wrote: >>>> PS: maybe better using function_declarator_p? >>> I think so, yes. The relevant rule seems to be "The declarator shall >>> not specify a function or an array.", so let's check for arrays, too. >> Agreed. I had the amended patch ready when I noticed (again) that it wasn't >> addressing another related class of issues which involves declarators not >> followed by initializers. Thus I tried to fix those too, and the below which >> moves the check up appears to work fine, passes testing, etc. Are there any >> risks that an erroneous function / array as declarator is in fact a well >> formed expression?!? > That doesn't matter; if it parses as a declarator, it's a declarator, > even if it's an ill-formed declarator. But... > > + bool decl_p = cp_parser_parse_definitely (parser); > + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) > + return error_mark_node; > > ...if cp_parser_parse_definitely returns false, parsing as a > declarator failed, so we shouldn't look at "declarator". Uhm, then you are saying that we should fix cp_parser_declarator itself, right? Because we don't want cp_parser_parse_definitely returning false after cp_parser_declarator parses, say, 'if (int foo())' and therefore cp_parser_condition proceed with cp_parser_expression, we want to emit our error and bail out. Therefore the problem in the new patch seems that it tries to paper over that cp_parser_declarator issue in the caller?!? Like, Ok, cp_parser_declarator failed, but it was anyway trying to declare a function / array and that can't possibly be an expression, see what I mean? *Somehow*, the question you answered above. Paolo.
Hi again, On 18/05/2018 02:31, Paolo Carlini wrote: > Hi, > > On 18/05/2018 01:21, Jason Merrill wrote: >> On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini >> <paolo.carlini@oracle.com> wrote: >>> On 17/05/2018 16:58, Jason Merrill wrote: >>>> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >>>> <paolo.carlini@oracle.com> wrote: >>>>> PS: maybe better using function_declarator_p? >>>> I think so, yes. The relevant rule seems to be "The declarator shall >>>> not specify a function or an array.", so let's check for arrays, too. >>> Agreed. I had the amended patch ready when I noticed (again) that it >>> wasn't >>> addressing another related class of issues which involves >>> declarators not >>> followed by initializers. Thus I tried to fix those too, and the >>> below which >>> moves the check up appears to work fine, passes testing, etc. Are >>> there any >>> risks that an erroneous function / array as declarator is in fact a >>> well >>> formed expression?!? >> That doesn't matter; if it parses as a declarator, it's a declarator, >> even if it's an ill-formed declarator. But... >> >> + bool decl_p = cp_parser_parse_definitely (parser); >> + if (!cp_parser_check_condition_declarator (parser, declarator, >> loc)) >> + return error_mark_node; >> >> ...if cp_parser_parse_definitely returns false, parsing as a >> declarator failed, so we shouldn't look at "declarator". > Uhm, then you are saying that we should fix cp_parser_declarator > itself, right? Because we don't want cp_parser_parse_definitely > returning false after cp_parser_declarator parses, say, 'if (int > foo())' and therefore cp_parser_condition proceed with > cp_parser_expression, we want to emit our error and bail out. > Therefore the problem in the new patch seems that it tries to paper > over that cp_parser_declarator issue in the caller?!? Like, Ok, > cp_parser_declarator failed, but it was anyway trying to declare a > function / array and that can't possibly be an expression, see what I > mean? *Somehow*, the question you answered above. Ok, now I finally see what's the exact issue you pointed out (I'm a bit tired). Seems fixable. If I understand correctly, the reason why the 3 lines you cited above are wrong as they are is that my patch *assumes* that cp_parser_declarator didn't really fail and cp_parser_condition has forced the tentative parse to fail by calling cp_parser_simulate_error immediately before when it didn't see an initializer immediately following. That's actually true for 'if (int foo())', thus it makes sense to check the declarator anyway for such cases *even* if cp_parser_parse_definitely returns false. See what I mean? Therefore, it seems to me that an amended patch would rearrange cp_parser_condition to *not* call cp_parser_simulate_error for the cases we care about ('if (int foo())') and instead check the declarator. I'll work on that tomorrow... Thanks, Paolo.
Hi, >> On 18/05/2018 01:21, Jason Merrill wrote: >>> On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini >>> <paolo.carlini@oracle.com> wrote: >>>> On 17/05/2018 16:58, Jason Merrill wrote: >>>>> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >>>>> <paolo.carlini@oracle.com> wrote: >>>>>> PS: maybe better using function_declarator_p? >>>>> I think so, yes. The relevant rule seems to be "The declarator shall >>>>> not specify a function or an array.", so let's check for arrays, too. >>>> Agreed. I had the amended patch ready when I noticed (again) that >>>> it wasn't >>>> addressing another related class of issues which involves >>>> declarators not >>>> followed by initializers. Thus I tried to fix those too, and the >>>> below which >>>> moves the check up appears to work fine, passes testing, etc. Are >>>> there any >>>> risks that an erroneous function / array as declarator is in fact a >>>> well >>>> formed expression?!? >>> That doesn't matter; if it parses as a declarator, it's a declarator, >>> even if it's an ill-formed declarator. But... >>> >>> + bool decl_p = cp_parser_parse_definitely (parser); >>> + if (!cp_parser_check_condition_declarator (parser, >>> declarator, loc)) >>> + return error_mark_node; >>> >>> ...if cp_parser_parse_definitely returns false, parsing as a >>> declarator failed, so we shouldn't look at "declarator". What I'm attaching below isn't affected by this problem: I moved the check further up - *before* maybe calling cp_parser_simulated_error because an initializer isn't in sight - and is carried out only when !cp_parser_error_occurred, thus cp_parser_declarator succeeded . cp_parser_commit_to_tentative_parse is called when the check fails. Bootstrapped and tested on x86_64-linux. Thanks! Paolo. ////////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 260347) +++ cp/parser.c (working copy) @@ -11527,6 +11527,34 @@ cp_parser_selection_statement (cp_parser* parser, } } +/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2: + The declarator shall not specify a function or an array. Returns + TRUE if the declarator is valid, FALSE otherwise. */ + +static bool +cp_parser_check_condition_declarator (cp_parser* parser, + cp_declarator *declarator, + location_t loc) +{ + if (function_declarator_p (declarator) + || declarator->kind == cdk_array) + { + cp_parser_commit_to_tentative_parse (parser); + if (declarator->kind == cdk_array) + error_at (loc, "condition declares an array"); + else + error_at (loc, "condition declares a function"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true, + /*or_comma=*/false, + /*consume_paren=*/false); + return false; + } + else + return true; +} + /* Parse a condition. condition: @@ -11571,6 +11599,7 @@ cp_parser_condition (cp_parser* parser) tree attributes; cp_declarator *declarator; tree initializer = NULL_TREE; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, @@ -11582,6 +11611,11 @@ cp_parser_condition (cp_parser* parser) attributes = cp_parser_attributes_opt (parser); /* Parse the asm-specification. */ asm_specification = cp_parser_asm_specification_opt (parser); + + if (!cp_parser_error_occurred (parser) + && !cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; + /* If the next token is not an `=' or '{', then we might still be looking at an expression. For example: Index: testsuite/g++.dg/cpp0x/cond1.C =================================================================== --- testsuite/g++.dg/cpp0x/cond1.C (nonexistent) +++ testsuite/g++.dg/cpp0x/cond1.C (working copy) @@ -0,0 +1,23 @@ +// PR c++/84588 +// { dg-do compile { target c++11 } } + +void foo() +{ + if (int bar() {}) // { dg-error "condition declares a function" } + ; + + for (;int bar() {};) // { dg-error "condition declares a function" } + ; + + while (int bar() {}) // { dg-error "condition declares a function" } + ; + + if (int a[] {}) // { dg-error "condition declares an array" } + ; + + for (;int a[] {};) // { dg-error "condition declares an array" } + ; + + while (int a[] {}) // { dg-error "condition declares an array" } + ; +} Index: testsuite/g++.dg/cpp1y/pr84588-1.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto) {};) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-2.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-2.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-2.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto);) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/parse/cond6.C =================================================================== --- testsuite/g++.dg/parse/cond6.C (nonexistent) +++ testsuite/g++.dg/parse/cond6.C (working copy) @@ -0,0 +1,22 @@ +// PR c++/84588 + +void foo() +{ + if (int bar()) // { dg-error "condition declares a function" } + ; + + for (;int bar();) // { dg-error "condition declares a function" } + ; + + while (int bar()) // { dg-error "condition declares a function" } + ; + + if (int a[]) // { dg-error "condition declares an array" } + ; + + for (;int a[];) // { dg-error "condition declares an array" } + ; + + while (int a[]) // { dg-error "condition declares an array" } + ; +} Index: testsuite/g++.old-deja/g++.jason/cond.C =================================================================== --- testsuite/g++.old-deja/g++.jason/cond.C (revision 260347) +++ testsuite/g++.old-deja/g++.jason/cond.C (working copy) @@ -47,11 +47,10 @@ int main() if (struct B * foo = new B) ; - if (int f () = 1) // { dg-warning "extern" "extern" } - // { dg-error "is initialized like a variable" "var" { target *-*-* } .-1 } + if (int f () = 1) // { dg-error "declares a function" } ; - if (int a[2] = {1, 2}) // { dg-error "extended init" "" { target { ! c++11 } } } + if (int a[2] = {1, 2}) // { dg-error "declares an array" } ; }
On Fri, May 18, 2018 at 4:41 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > >>> On 18/05/2018 01:21, Jason Merrill wrote: >>>> >>>> On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini >>>> <paolo.carlini@oracle.com> wrote: >>>>> >>>>> On 17/05/2018 16:58, Jason Merrill wrote: >>>>>> >>>>>> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >>>>>> <paolo.carlini@oracle.com> wrote: >>>>>>> >>>>>>> PS: maybe better using function_declarator_p? >>>>>> >>>>>> I think so, yes. The relevant rule seems to be "The declarator shall >>>>>> not specify a function or an array.", so let's check for arrays, too. >>>>> >>>>> Agreed. I had the amended patch ready when I noticed (again) that it >>>>> wasn't >>>>> addressing another related class of issues which involves declarators >>>>> not >>>>> followed by initializers. Thus I tried to fix those too, and the below >>>>> which >>>>> moves the check up appears to work fine, passes testing, etc. Are there >>>>> any >>>>> risks that an erroneous function / array as declarator is in fact a >>>>> well >>>>> formed expression?!? >>>> >>>> That doesn't matter; if it parses as a declarator, it's a declarator, >>>> even if it's an ill-formed declarator. But... >>>> >>>> + bool decl_p = cp_parser_parse_definitely (parser); >>>> + if (!cp_parser_check_condition_declarator (parser, declarator, >>>> loc)) >>>> + return error_mark_node; >>>> >>>> ...if cp_parser_parse_definitely returns false, parsing as a >>>> declarator failed, so we shouldn't look at "declarator". > > What I'm attaching below isn't affected by this problem: I moved the check > further up - *before* maybe calling cp_parser_simulated_error because an > initializer isn't in sight - and is carried out only when > !cp_parser_error_occurred, thus cp_parser_declarator succeeded . > cp_parser_commit_to_tentative_parse is called when the check fails. > Bootstrapped and tested on x86_64-linux. I had in mind moving the call to cp_parser_check_condition_declarator into the block controlled by cp_parser_parse_definitely; this is a semantic check that should follow the syntactic checks. If there's no initializer, it doesn't parse as a condition declaration, so we don't want to complain about it being a semantically invalid condition declaration. Jason
Hi, On 18/05/2018 15:56, Jason Merrill wrote: > I had in mind moving the call to cp_parser_check_condition_declarator > into the block controlled by cp_parser_parse_definitely; this is a > semantic check that should follow the syntactic checks. If there's no > initializer, it doesn't parse as a condition declaration, so we don't > want to complain about it being a semantically invalid condition > declaration. If we do that we are back to something very, very, similar to what I posted at the beginning of the thread, right? Therefore, for all the testcases which don't have an initializer we end-up with *horrible*, literally *horrible* cascades of errors, plus we ICE on the c++/84588 variants without an initializer. If you like we can do that and defer the other related issues - strictly speaking c++/84588 would be fixed - to another new bug. Paolo.
On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > On 18/05/2018 15:56, Jason Merrill wrote: >> >> I had in mind moving the call to cp_parser_check_condition_declarator >> into the block controlled by cp_parser_parse_definitely; this is a >> semantic check that should follow the syntactic checks. If there's no >> initializer, it doesn't parse as a condition declaration, so we don't >> want to complain about it being a semantically invalid condition >> declaration. > > If we do that we are back to something very, very, similar to what I posted > at the beginning of the thread, right? Therefore, for all the testcases > which don't have an initializer we end-up with *horrible*, literally > *horrible* cascades of errors, plus we ICE on the c++/84588 variants without > an initializer. Ah. Why is that? I guess it's desirable to also give this error when the declarator is followed by ) or ; rather than other tokens that could be more expression (like in A(a).x in the comment). Jason
Hi, On 18/05/2018 16:19, Jason Merrill wrote: > On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini > <paolo.carlini@oracle.com> wrote: >> Hi, >> >> On 18/05/2018 15:56, Jason Merrill wrote: >>> I had in mind moving the call to cp_parser_check_condition_declarator >>> into the block controlled by cp_parser_parse_definitely; this is a >>> semantic check that should follow the syntactic checks. If there's no >>> initializer, it doesn't parse as a condition declaration, so we don't >>> want to complain about it being a semantically invalid condition >>> declaration. >> If we do that we are back to something very, very, similar to what I posted >> at the beginning of the thread, right? Therefore, for all the testcases >> which don't have an initializer we end-up with *horrible*, literally >> *horrible* cascades of errors, plus we ICE on the c++/84588 variants without >> an initializer. > Ah. Why is that? > > I guess it's desirable to also give this error when the declarator is > followed by ) or ; rather than other tokens that could be more > expression (like in A(a).x in the comment). I can certainly try to implement this, maybe just something minimal to begin with, as you say ) or ; alone would be safe and would already take care of all the tests I have around. Certainly, unconditionally deferring at that point to cp_parser_expression leads to *very* bad error-recovery. For fun, try with 8.1.0: void foo() { for (;void bar();); } and it gets worse. Paolo.
On Fri, May 18, 2018 at 10:30 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > On 18/05/2018 16:19, Jason Merrill wrote: >> >> On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini >> <paolo.carlini@oracle.com> wrote: >>> >>> Hi, >>> >>> On 18/05/2018 15:56, Jason Merrill wrote: >>>> >>>> I had in mind moving the call to cp_parser_check_condition_declarator >>>> into the block controlled by cp_parser_parse_definitely; this is a >>>> semantic check that should follow the syntactic checks. If there's no >>>> initializer, it doesn't parse as a condition declaration, so we don't >>>> want to complain about it being a semantically invalid condition >>>> declaration. >>> >>> If we do that we are back to something very, very, similar to what I >>> posted >>> at the beginning of the thread, right? Therefore, for all the testcases >>> which don't have an initializer we end-up with *horrible*, literally >>> *horrible* cascades of errors, plus we ICE on the c++/84588 variants >>> without >>> an initializer. >> >> Ah. Why is that? >> >> I guess it's desirable to also give this error when the declarator is >> followed by ) or ; rather than other tokens that could be more >> expression (like in A(a).x in the comment). > > I can certainly try to implement this, maybe just something minimal to begin > with, as you say ) or ; alone would be safe and would already take care of > all the tests I have around. > > Certainly, unconditionally deferring at that point to cp_parser_expression > leads to *very* bad error-recovery. For fun, try with 8.1.0: > > void foo() > { > for (;void bar();); > } > > and it gets worse. It would also need to be only for a non-parenthesized declarator, which can't be an expression; a parenthesized declarator can be, as in this well-formed testcase: bool (bar()) { return 0; } // declaration void foo() { for (;bool (bar());); // expression } Jason
Hi, On 18/05/2018 16:45, Jason Merrill wrote: >>> I guess it's desirable to also give this error when the declarator is >>> followed by ) or ; rather than other tokens that could be more >>> expression (like in A(a).x in the comment). >> I can certainly try to implement this, maybe just something minimal to begin >> with, as you say ) or ; alone would be safe and would already take care of >> all the tests I have around. >> >> Certainly, unconditionally deferring at that point to cp_parser_expression >> leads to *very* bad error-recovery. For fun, try with 8.1.0: >> >> void foo() >> { >> for (;void bar();); >> } >> >> and it gets worse. > It would also need to be only for a non-parenthesized declarator, > which can't be an expression; a parenthesized declarator can be, as in > this well-formed testcase: > > bool (bar()) { return 0; } // declaration > > void foo() > { > for (;bool (bar());); // expression > } Thanks for clarifying that. I'll make sure we have such testcases. Interestingly, if we aren't careful about that in the new code, libstdc++-v3 doesn't even build, eh! Paolo.
Hi again,
I'm playing with a wild, wild idea: would it make sense to try *first*
an expression? Because, a quickly hacked draft appears to handle very
elegantly all the possible cases of "junk" after the declarator, eg:
void foo()
{
if (void bar()JUNK);
}
and the parenthesized case, etc, etc. Before trying to seriously work on
that I wanted to ask...
Paolo.
On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi again, > > I'm playing with a wild, wild idea: would it make sense to try *first* an > expression? Because, a quickly hacked draft appears to handle very elegantly > all the possible cases of "junk" after the declarator, eg: > > void foo() > { > if (void bar()JUNK); > } > > and the parenthesized case, etc, etc. Before trying to seriously work on > that I wanted to ask... We'd need to try parsing as a declaration whether or not parsing as an expression works, since any ambiguous cases are treated as declarations [stmt.ambig]. Jason
Hi, On 19/05/2018 01:40, Jason Merrill wrote: > On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> Hi again, >> >> I'm playing with a wild, wild idea: would it make sense to try *first* an >> expression? Because, a quickly hacked draft appears to handle very elegantly >> all the possible cases of "junk" after the declarator, eg: >> >> void foo() >> { >> if (void bar()JUNK); >> } >> >> and the parenthesized case, etc, etc. Before trying to seriously work on >> that I wanted to ask... > We'd need to try parsing as a declaration whether or not parsing as an > expression works, since any ambiguous cases are treated as > declarations [stmt.ambig]. Yeah, that complicates the implementation of my idea. However, I'm thinking that at least *in the specific case of cp_parse_condition* from the computational complexity point of view probably we wouldn't regress much, because declarations are rare anyway, thus in most of the cases we end up doing both anyway. If we do expressions first and we save the result, then I believe when we can handle the declarator as something which cannot be a function or an array even if we don't see the initializer much more easily, we easily have a better diagnostic for things like if (int x); (another case we currently handle pretty badly, we don't talk about the missing initializer at all!), we cope elegantly with any junk following the wrong function/array declaration, etc. See that attached, if you are curious, which essentially passes the testsuite modulo a nit (*) which doesn't have anything to do with [stmt.ambig] per se (which of course is *not* correctly implemented in the wip). Can you give me your opinion about the more detailed idea, in particular whether we already have good infrastructure to implement [stmt.ambig] in this context, thus to keep the first parsing as an expression around, possibly returning to it if the parsing as a declaration fails?? I hope I'm making sense, it's again late here... in any case the wip patch I did much earlier today ;) Paolo. (*) Has to do with David's access failure hint not handling deferred access checks (accessor-fixits-6.C). Index: parser.c =================================================================== --- parser.c (revision 260347) +++ parser.c (working copy) @@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser, } } +/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2: + The declarator shall not specify a function or an array. Returns + TRUE if the declarator is valid, FALSE otherwise. */ + +static bool +cp_parser_check_condition_declarator (cp_parser* parser, + cp_declarator *declarator, + location_t loc) +{ + if (function_declarator_p (declarator) + || declarator->kind == cdk_array) + { + if (declarator->kind == cdk_array) + error_at (loc, "condition declares an array"); + else + error_at (loc, "condition declares a function"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true, + /*or_comma=*/false, + /*consume_paren=*/false); + return false; + } + else + return true; +} + /* Parse a condition. condition: @@ -11545,12 +11572,20 @@ cp_parser_selection_statement (cp_parser* parser, static tree cp_parser_condition (cp_parser* parser) { + cp_parser_parse_tentatively (parser); + + /* Try the expression first. */ + tree expr = cp_parser_expression (parser); + + if (cp_parser_parse_definitely (parser)) + return expr; + + cp_parser_parse_tentatively (parser); + cp_decl_specifier_seq type_specifiers; const char *saved_message; int declares_class_or_enum; - /* Try the declaration first. */ - cp_parser_parse_tentatively (parser); /* New types are not allowed in the type-specifier-seq for a condition. */ saved_message = parser->type_definition_forbidden_message; @@ -11563,6 +11598,7 @@ cp_parser_condition (cp_parser* parser) &declares_class_or_enum); /* Restore the saved message. */ parser->type_definition_forbidden_message = saved_message; + /* If all is well, we might be looking at a declaration. */ if (!cp_parser_error_occurred (parser)) { @@ -11570,7 +11606,8 @@ cp_parser_condition (cp_parser* parser) tree asm_specification; tree attributes; cp_declarator *declarator; - tree initializer = NULL_TREE; + tree initializer; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, @@ -11582,19 +11619,7 @@ cp_parser_condition (cp_parser* parser) attributes = cp_parser_attributes_opt (parser); /* Parse the asm-specification. */ asm_specification = cp_parser_asm_specification_opt (parser); - /* If the next token is not an `=' or '{', then we might still be - looking at an expression. For example: - if (A(a).x) - - looks like a decl-specifier-seq and a declarator -- but then - there is no `=', so this is an expression. */ - if (cp_lexer_next_token_is_not (parser->lexer, CPP_EQ) - && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) - cp_parser_simulate_error (parser); - - /* If we did see an `=' or '{', then we are looking at a declaration - for sure. */ if (cp_parser_parse_definitely (parser)) { tree pushed_scope; @@ -11601,6 +11626,9 @@ cp_parser_condition (cp_parser* parser) bool non_constant_p; int flags = LOOKUP_ONLYCONVERTING; + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; + /* Create the declaration. */ decl = start_decl (declarator, &type_specifiers, /*initialized_p=*/true, @@ -11614,12 +11642,18 @@ cp_parser_condition (cp_parser* parser) CONSTRUCTOR_IS_DIRECT_INIT (initializer) = 1; flags = 0; } - else + else if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) { /* Consume the `='. */ cp_parser_require (parser, CPP_EQ, RT_EQ); - initializer = cp_parser_initializer_clause (parser, &non_constant_p); + initializer = cp_parser_initializer_clause (parser, + &non_constant_p); } + else + { + cp_parser_error (parser, "expected initializer"); + initializer = error_mark_node; + } if (BRACE_ENCLOSED_INITIALIZER_P (initializer)) maybe_warn_cpp0x (CPP0X_INITIALIZER_LISTS); @@ -11640,8 +11674,8 @@ cp_parser_condition (cp_parser* parser) else cp_parser_abort_tentative_parse (parser); - /* Otherwise, we are looking at an expression. */ - return cp_parser_expression (parser); + cp_parser_error (parser, "expected condition"); + return error_mark_node; } /* Parses a for-statement or range-for-statement until the closing ')',
On Fri, May 18, 2018 at 8:27 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > On 19/05/2018 01:40, Jason Merrill wrote: >> On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini <paolo.carlini@oracle.com> >> wrote: >>> >>> Hi again, >>> >>> I'm playing with a wild, wild idea: would it make sense to try *first* an >>> expression? Because, a quickly hacked draft appears to handle very >>> elegantly >>> all the possible cases of "junk" after the declarator, eg: >>> >>> void foo() >>> { >>> if (void bar()JUNK); >>> } >>> >>> and the parenthesized case, etc, etc. Before trying to seriously work on >>> that I wanted to ask... >> >> We'd need to try parsing as a declaration whether or not parsing as an >> expression works, since any ambiguous cases are treated as >> declarations [stmt.ambig]. > > Yeah, that complicates the implementation of my idea. However, I'm thinking > that at least *in the specific case of cp_parse_condition* from the > computational complexity point of view probably we wouldn't regress much, > because declarations are rare anyway, thus in most of the cases we end up > doing both anyway. If we do expressions first and we save the result, then I > believe when we can handle the declarator as something which cannot be a > function or an array even if we don't see the initializer much more easily, > we easily have a better diagnostic for things like > > if (int x); > > (another case we currently handle pretty badly, we don't talk about the > missing initializer at all!), we cope elegantly with any junk following the > wrong function/array declaration, etc. See that attached, if you are > curious, which essentially passes the testsuite modulo a nit (*) which > doesn't have anything to do with [stmt.ambig] per se (which of course is > *not* correctly implemented in the wip). > > Can you give me your opinion about the more detailed idea, in particular > whether we already have good infrastructure to implement [stmt.ambig] in > this context, thus to keep the first parsing as an expression around, > possibly returning to it if the parsing as a declaration fails?? I would expect it to cause different diagnostic issues, from complaining about something not being a proper declaration when it's really an expression. I also wonder about warning problems (either missed or bogus) due to trying these in a different order. How about doing cp_parser_commit_to_tentative_parse if we see something that must be a declaration? cp_parser_simple_declaration has /* If we have seen at least one decl-specifier, and the next token is not a parenthesis, then we must be looking at a declaration. (After "int (" we might be looking at a functional cast.) */ if (decl_specifiers.any_specifiers_p && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) && !cp_parser_error_occurred (parser)) cp_parser_commit_to_tentative_parse (parser); That seems useful here, as well. Maybe factored into a separate function. Jason
Hi, On 19/05/2018 15:30, Jason Merrill wrote: > I would expect it to cause different diagnostic issues, from > complaining about something not being a proper declaration when it's > really an expression. I also wonder about warning problems (either > missed or bogus) due to trying these in a different order. Yes. It seems kind of science-fiction project ;) I'll give it more thought, anyway... > How about doing cp_parser_commit_to_tentative_parse if we see > something that must be a declaration? cp_parser_simple_declaration > has > > /* If we have seen at least one decl-specifier, and the next token > is not a parenthesis, then we must be looking at a declaration. > (After "int (" we might be looking at a functional cast.) */ > if (decl_specifiers.any_specifiers_p > && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) > && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) > && !cp_parser_error_occurred (parser)) > cp_parser_commit_to_tentative_parse (parser); > > That seems useful here, as well. Maybe factored into a separate function. Hmm, I see, thanks for the tip. To other day, eventually I figured out the below and fully tested it, which seems quite good to me, frankly: it simply adds a check of parenthesized_p (per your highly welcome previous clarification) to the existing checks: having spent quite a bit of time on [dcl.ambig.res] I think it's Ok - if the declarator isn't parenthesized can't be in fact an expression - and alone that allows us to make very good progress - well beyond the requirements of c++/84588 - on a number of diagnostic-quality fronts, see the attached additional testcases too. I can still construct a nasty condition of the form, say, 'a (a(int auto)JUNK' which we don't handle well in terms of error recovery, but if you agree that the below is correct, it's probably enough for the *regression* part... Thanks! Paolo. ///////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 260402) +++ cp/parser.c (working copy) @@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser, } } +/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2: + The declarator shall not specify a function or an array. Returns + TRUE if the declarator is valid, FALSE otherwise. */ + +static bool +cp_parser_check_condition_declarator (cp_parser* parser, + cp_declarator *declarator, + location_t loc) +{ + if (function_declarator_p (declarator) + || declarator->kind == cdk_array) + { + if (declarator->kind == cdk_array) + error_at (loc, "condition declares an array"); + else + error_at (loc, "condition declares a function"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true, + /*or_comma=*/false, + /*consume_paren=*/false); + return false; + } + else + return true; +} + /* Parse a condition. condition: @@ -11571,11 +11598,13 @@ cp_parser_condition (cp_parser* parser) tree attributes; cp_declarator *declarator; tree initializer = NULL_TREE; + bool parenthesized_p = false; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, /*ctor_dtor_or_conv_p=*/NULL, - /*parenthesized_p=*/NULL, + &parenthesized_p, /*member_p=*/false, /*friend_p=*/false); /* Parse the attributes. */ @@ -11582,8 +11611,9 @@ cp_parser_condition (cp_parser* parser) attributes = cp_parser_attributes_opt (parser); /* Parse the asm-specification. */ asm_specification = cp_parser_asm_specification_opt (parser); - /* If the next token is not an `=' or '{', then we might still be - looking at an expression. For example: + /* If the next token is not an `=' or '{' and the declarator is + parenthesized, then we might still be looking at an expression. + For example: if (A(a).x) @@ -11590,11 +11620,12 @@ cp_parser_condition (cp_parser* parser) looks like a decl-specifier-seq and a declarator -- but then there is no `=', so this is an expression. */ if (cp_lexer_next_token_is_not (parser->lexer, CPP_EQ) - && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)) + && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) + && parenthesized_p) cp_parser_simulate_error (parser); - /* If we did see an `=' or '{', then we are looking at a declaration - for sure. */ + /* If we did see an `=' or '{' or the declarator isn't parenthesized, + then we are looking at a declaration for sure. */ if (cp_parser_parse_definitely (parser)) { tree pushed_scope; @@ -11601,6 +11632,9 @@ cp_parser_condition (cp_parser* parser) bool non_constant_p; int flags = LOOKUP_ONLYCONVERTING; + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; + /* Create the declaration. */ decl = start_decl (declarator, &type_specifiers, /*initialized_p=*/true, @@ -11614,12 +11648,18 @@ cp_parser_condition (cp_parser* parser) CONSTRUCTOR_IS_DIRECT_INIT (initializer) = 1; flags = 0; } - else + else if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) { /* Consume the `='. */ - cp_parser_require (parser, CPP_EQ, RT_EQ); - initializer = cp_parser_initializer_clause (parser, &non_constant_p); + cp_lexer_consume_token (parser->lexer); + initializer = cp_parser_initializer_clause (parser, + &non_constant_p); } + else + { + cp_parser_error (parser, "expected initializer"); + initializer = error_mark_node; + } if (BRACE_ENCLOSED_INITIALIZER_P (initializer)) maybe_warn_cpp0x (CPP0X_INITIALIZER_LISTS); Index: testsuite/g++.dg/cpp0x/cond1.C =================================================================== --- testsuite/g++.dg/cpp0x/cond1.C (nonexistent) +++ testsuite/g++.dg/cpp0x/cond1.C (working copy) @@ -0,0 +1,17 @@ +// PR c++/84588 +// { dg-do compile { target c++11 } } + +void foo() +{ + if (int bar() {}); // { dg-error "condition declares a function" } + + for (;int bar() {};); // { dg-error "condition declares a function" } + + while (int bar() {}); // { dg-error "condition declares a function" } + + if (int a[] {}); // { dg-error "condition declares an array" } + + for (;int a[] {};); // { dg-error "condition declares an array" } + + while (int a[] {}); // { dg-error "condition declares an array" } +} Index: testsuite/g++.dg/cpp1y/pr84588-1.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto) {};) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-2.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-2.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-2.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto);) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-3.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-3.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-3.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)JUNK) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto)JUNK;) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)JUNK) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/parse/cond6.C =================================================================== --- testsuite/g++.dg/parse/cond6.C (nonexistent) +++ testsuite/g++.dg/parse/cond6.C (working copy) @@ -0,0 +1,16 @@ +// PR c++/84588 + +void foo() +{ + if (int bar()); // { dg-error "condition declares a function" } + + for (;int bar();); // { dg-error "condition declares a function" } + + while (int bar()); // { dg-error "condition declares a function" } + + if (int a[]); // { dg-error "condition declares an array" } + + for (;int a[];); // { dg-error "condition declares an array" } + + while (int a[]); // { dg-error "condition declares an array" } +} Index: testsuite/g++.dg/parse/cond7.C =================================================================== --- testsuite/g++.dg/parse/cond7.C (nonexistent) +++ testsuite/g++.dg/parse/cond7.C (working copy) @@ -0,0 +1,18 @@ +// PR c++/84588 + +bool (bar()) { return 0; } // declaration + +void foo1() +{ + if (bool (bar())); // expression +} + +void foo2() +{ + for (;bool (bar());); // expression +} + +void foo3() +{ + while (bool (bar())); // expression +} Index: testsuite/g++.dg/parse/cond8.C =================================================================== --- testsuite/g++.dg/parse/cond8.C (nonexistent) +++ testsuite/g++.dg/parse/cond8.C (working copy) @@ -0,0 +1,16 @@ +// PR c++/84588 + +void foo() +{ + if (int x); // { dg-error "expected initializer" } + + for (;int x;); // { dg-error "expected initializer" } + + while (int x); // { dg-error "expected initializer" } + + if (int x); // { dg-error "expected initializer" } + + for (;int x;); // { dg-error "expected initializer" } + + while (int x); // { dg-error "expected initializer" } +} Index: testsuite/g++.old-deja/g++.jason/cond.C =================================================================== --- testsuite/g++.old-deja/g++.jason/cond.C (revision 260402) +++ testsuite/g++.old-deja/g++.jason/cond.C (working copy) @@ -47,11 +47,10 @@ int main() if (struct B * foo = new B) ; - if (int f () = 1) // { dg-warning "extern" "extern" } - // { dg-error "is initialized like a variable" "var" { target *-*-* } .-1 } + if (int f () = 1) // { dg-error "declares a function" } ; - if (int a[2] = {1, 2}) // { dg-error "extended init" "" { target { ! c++11 } } } + if (int a[2] = {1, 2}) // { dg-error "declares an array" } ; }
Hi again, On 19/05/2018 15:30, Jason Merrill wrote: > How about doing cp_parser_commit_to_tentative_parse if we see > something that must be a declaration? cp_parser_simple_declaration > has > > /* If we have seen at least one decl-specifier, and the next token > is not a parenthesis, then we must be looking at a declaration. > (After "int (" we might be looking at a functional cast.) */ > if (decl_specifiers.any_specifiers_p > && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) > && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) > && !cp_parser_error_occurred (parser)) > cp_parser_commit_to_tentative_parse (parser); > > That seems useful here, as well. Maybe factored into a separate function. The below implements this new idea, which indeed appears to work well: I tested it and testsuite-wise seems essentially equivalent to what I posted yesterday, besides a slightly worse error-recovery for the first issue in cpp1z/decomp16.C: an additional 'no match for ‘operator=’' error. Thanks! Paolo. ////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 260402) +++ cp/parser.c (working copy) @@ -11527,6 +11527,53 @@ cp_parser_selection_statement (cp_parser* parser, } } +/* Helper function for cp_parser_condition and cp_parser_simple_declaration. + If we have seen at least one decl-specifier, and the next token + is not a parenthesis, then we must be looking at a declaration. + (After "int (" we might be looking at a functional cast.) */ + +static bool +cp_parser_maybe_commit_to_declaration (cp_parser* parser, + bool any_specifiers_p) +{ + if (any_specifiers_p + && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) + && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) + && !cp_parser_error_occurred (parser)) + { + cp_parser_commit_to_tentative_parse (parser); + return true; + } + return false; +} + +/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2: + The declarator shall not specify a function or an array. Returns + TRUE if the declarator is valid, FALSE otherwise. */ + +static bool +cp_parser_check_condition_declarator (cp_parser* parser, + cp_declarator *declarator, + location_t loc) +{ + if (function_declarator_p (declarator) + || declarator->kind == cdk_array) + { + if (declarator->kind == cdk_array) + error_at (loc, "condition declares an array"); + else + error_at (loc, "condition declares a function"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true, + /*or_comma=*/false, + /*consume_paren=*/false); + return false; + } + else + return true; +} + /* Parse a condition. condition: @@ -11563,6 +11610,10 @@ cp_parser_condition (cp_parser* parser) &declares_class_or_enum); /* Restore the saved message. */ parser->type_definition_forbidden_message = saved_message; + + cp_parser_maybe_commit_to_declaration (parser, + type_specifiers.any_specifiers_p); + /* If all is well, we might be looking at a declaration. */ if (!cp_parser_error_occurred (parser)) { @@ -11571,6 +11622,7 @@ cp_parser_condition (cp_parser* parser) tree attributes; cp_declarator *declarator; tree initializer = NULL_TREE; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, @@ -11601,6 +11653,9 @@ cp_parser_condition (cp_parser* parser) bool non_constant_p; int flags = LOOKUP_ONLYCONVERTING; + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) + return error_mark_node; + /* Create the declaration. */ decl = start_decl (declarator, &type_specifiers, /*initialized_p=*/true, @@ -11614,12 +11669,18 @@ cp_parser_condition (cp_parser* parser) CONSTRUCTOR_IS_DIRECT_INIT (initializer) = 1; flags = 0; } - else + else if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) { /* Consume the `='. */ - cp_parser_require (parser, CPP_EQ, RT_EQ); - initializer = cp_parser_initializer_clause (parser, &non_constant_p); + cp_lexer_consume_token (parser->lexer); + initializer = cp_parser_initializer_clause (parser, + &non_constant_p); } + else + { + cp_parser_error (parser, "expected initializer"); + initializer = error_mark_node; + } if (BRACE_ENCLOSED_INITIALIZER_P (initializer)) maybe_warn_cpp0x (CPP0X_INITIALIZER_LISTS); @@ -12936,14 +12997,8 @@ cp_parser_simple_declaration (cp_parser* parser, goto done; } - /* If we have seen at least one decl-specifier, and the next token - is not a parenthesis, then we must be looking at a declaration. - (After "int (" we might be looking at a functional cast.) */ - if (decl_specifiers.any_specifiers_p - && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) - && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) - && !cp_parser_error_occurred (parser)) - cp_parser_commit_to_tentative_parse (parser); + cp_parser_maybe_commit_to_declaration (parser, + decl_specifiers.any_specifiers_p); /* Look for C++17 decomposition declaration. */ for (size_t n = 1; ; n++) Index: testsuite/g++.dg/cpp0x/cond1.C =================================================================== --- testsuite/g++.dg/cpp0x/cond1.C (nonexistent) +++ testsuite/g++.dg/cpp0x/cond1.C (working copy) @@ -0,0 +1,17 @@ +// PR c++/84588 +// { dg-do compile { target c++11 } } + +void foo() +{ + if (int bar() {}); // { dg-error "condition declares a function" } + + for (;int bar() {};); // { dg-error "condition declares a function" } + + while (int bar() {}); // { dg-error "condition declares a function" } + + if (int a[] {}); // { dg-error "condition declares an array" } + + for (;int a[] {};); // { dg-error "condition declares an array" } + + while (int a[] {}); // { dg-error "condition declares an array" } +} Index: testsuite/g++.dg/cpp1y/pr84588-1.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto) {};) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto) {}) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-2.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-2.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-2.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto);) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1y/pr84588-3.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588-3.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588-3.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto)JUNK) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto)JUNK;) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto)JUNK) // { dg-error "two or more data types|condition declares a function" } + ; + }) {} +}; Index: testsuite/g++.dg/cpp1z/decomp16.C =================================================================== --- testsuite/g++.dg/cpp1z/decomp16.C (revision 260402) +++ testsuite/g++.dg/cpp1z/decomp16.C (working copy) @@ -8,7 +8,7 @@ void foo () { auto [ a, b ] = A (); - for (; auto [ a, b ] = A (); ) // { dg-error "expected" } + for (; auto [ a, b ] = A (); ) // { dg-error "expected|no match" } ; for (; false; auto [ a, b ] = A ()) // { dg-error "expected" } ; Index: testsuite/g++.dg/parse/cond6.C =================================================================== --- testsuite/g++.dg/parse/cond6.C (nonexistent) +++ testsuite/g++.dg/parse/cond6.C (working copy) @@ -0,0 +1,16 @@ +// PR c++/84588 + +void foo() +{ + if (int bar()); // { dg-error "condition declares a function" } + + for (;int bar();); // { dg-error "condition declares a function" } + + while (int bar()); // { dg-error "condition declares a function" } + + if (int a[]); // { dg-error "condition declares an array" } + + for (;int a[];); // { dg-error "condition declares an array" } + + while (int a[]); // { dg-error "condition declares an array" } +} Index: testsuite/g++.dg/parse/cond7.C =================================================================== --- testsuite/g++.dg/parse/cond7.C (nonexistent) +++ testsuite/g++.dg/parse/cond7.C (working copy) @@ -0,0 +1,18 @@ +// PR c++/84588 + +bool (bar()) { return 0; } // declaration + +void foo1() +{ + if (bool (bar())); // expression +} + +void foo2() +{ + for (;bool (bar());); // expression +} + +void foo3() +{ + while (bool (bar())); // expression +} Index: testsuite/g++.dg/parse/cond8.C =================================================================== --- testsuite/g++.dg/parse/cond8.C (nonexistent) +++ testsuite/g++.dg/parse/cond8.C (working copy) @@ -0,0 +1,10 @@ +// PR c++/84588 + +void foo() +{ + if (int x); // { dg-error "expected initializer" } + + for (;int x;); // { dg-error "expected initializer" } + + while (int x); // { dg-error "expected initializer" } +} Index: testsuite/g++.old-deja/g++.jason/cond.C =================================================================== --- testsuite/g++.old-deja/g++.jason/cond.C (revision 260402) +++ testsuite/g++.old-deja/g++.jason/cond.C (working copy) @@ -47,11 +47,10 @@ int main() if (struct B * foo = new B) ; - if (int f () = 1) // { dg-warning "extern" "extern" } - // { dg-error "is initialized like a variable" "var" { target *-*-* } .-1 } + if (int f () = 1) // { dg-error "declares a function" } ; - if (int a[2] = {1, 2}) // { dg-error "extended init" "" { target { ! c++11 } } } + if (int a[2] = {1, 2}) // { dg-error "declares an array" } ; }
OK. On Mon, May 21, 2018 at 8:41 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi again, > > On 19/05/2018 15:30, Jason Merrill wrote: >> >> How about doing cp_parser_commit_to_tentative_parse if we see >> something that must be a declaration? cp_parser_simple_declaration >> has >> >> /* If we have seen at least one decl-specifier, and the next token >> is not a parenthesis, then we must be looking at a declaration. >> (After "int (" we might be looking at a functional cast.) */ >> if (decl_specifiers.any_specifiers_p >> && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) >> && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) >> && !cp_parser_error_occurred (parser)) >> cp_parser_commit_to_tentative_parse (parser); >> >> That seems useful here, as well. Maybe factored into a separate function. > > The below implements this new idea, which indeed appears to work well: I > tested it and testsuite-wise seems essentially equivalent to what I posted > yesterday, besides a slightly worse error-recovery for the first issue in > cpp1z/decomp16.C: an additional 'no match for ‘operator=’' error. > > Thanks! > Paolo. > > ////////////////// >
On Mon, May 21, 2018 at 12:13 PM, Jason Merrill <jason@redhat.com> wrote: > On Mon, May 21, 2018 at 8:41 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> On 19/05/2018 15:30, Jason Merrill wrote: >>> How about doing cp_parser_commit_to_tentative_parse if we see >>> something that must be a declaration? cp_parser_simple_declaration >>> has >>> >>> /* If we have seen at least one decl-specifier, and the next token >>> is not a parenthesis, then we must be looking at a declaration. >>> (After "int (" we might be looking at a functional cast.) */ >>> if (decl_specifiers.any_specifiers_p >>> && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN) >>> && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE) >>> && !cp_parser_error_occurred (parser)) >>> cp_parser_commit_to_tentative_parse (parser); >>> >>> That seems useful here, as well. Maybe factored into a separate function. >> >> The below implements this new idea, which indeed appears to work well: I >> tested it and testsuite-wise seems essentially equivalent to what I posted >> yesterday, besides a slightly worse error-recovery for the first issue in >> cpp1z/decomp16.C: an additional 'no match for ‘operator=’' error. I notice that we can improve error-recovery for decomp16.C by checking for cp_error_declarator as well. Tested x86_64-pc-linux-gnu, applying to trunkk. commit d06d67adb01349f4ba9ddf2572c3efcea86741f5 Author: Jason Merrill <jason@redhat.com> Date: Tue May 29 18:18:31 2018 -0400 Improve error recovery for structured binding in condition. * parser.c (cp_parser_check_condition_declarator): Handle cp_error_declarator. diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index d17beb8f930..de090d42d8d 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -11594,10 +11594,13 @@ cp_parser_check_condition_declarator (cp_parser* parser, cp_declarator *declarator, location_t loc) { - if (function_declarator_p (declarator) + if (declarator == cp_error_declarator + || function_declarator_p (declarator) || declarator->kind == cdk_array) { - if (declarator->kind == cdk_array) + if (declarator == cp_error_declarator) + /* Already complained. */; + else if (declarator->kind == cdk_array) error_at (loc, "condition declares an array"); else error_at (loc, "condition declares a function"); diff --git a/gcc/testsuite/g++.dg/cpp1z/decomp16.C b/gcc/testsuite/g++.dg/cpp1z/decomp16.C index dad69b89c08..7589c8015a5 100644 --- a/gcc/testsuite/g++.dg/cpp1z/decomp16.C +++ b/gcc/testsuite/g++.dg/cpp1z/decomp16.C @@ -8,7 +8,7 @@ void foo () { auto [ a, b ] = A (); - for (; auto [ a, b ] = A (); ) // { dg-error "expected|no match" } + for (; auto [ a, b ] = A (); ) // { dg-error "expected" } ; for (; false; auto [ a, b ] = A ()) // { dg-error "expected" } ;
Index: cp/parser.c =================================================================== --- cp/parser.c (revision 260308) +++ cp/parser.c (working copy) @@ -11571,6 +11571,7 @@ cp_parser_condition (cp_parser* parser) tree attributes; cp_declarator *declarator; tree initializer = NULL_TREE; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; /* Parse the declarator. */ declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED, @@ -11597,15 +11598,23 @@ cp_parser_condition (cp_parser* parser) for sure. */ if (cp_parser_parse_definitely (parser)) { - tree pushed_scope; + tree pushed_scope = NULL_TREE; bool non_constant_p; int flags = LOOKUP_ONLYCONVERTING; /* Create the declaration. */ - decl = start_decl (declarator, &type_specifiers, - /*initialized_p=*/true, - attributes, /*prefix_attributes=*/NULL_TREE, - &pushed_scope); + if (declarator->kind == cdk_function) + { + error_at (loc, "a function type is not allowed here"); + if (parser->fully_implicit_function_template_p) + abort_fully_implicit_template (parser); + decl = error_mark_node; + } + else + decl = start_decl (declarator, &type_specifiers, + /*initialized_p=*/true, + attributes, /*prefix_attributes=*/NULL_TREE, + &pushed_scope); /* Parse the initializer. */ if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)) Index: testsuite/g++.dg/cpp1y/pr84588.C =================================================================== --- testsuite/g++.dg/cpp1y/pr84588.C (nonexistent) +++ testsuite/g++.dg/cpp1y/pr84588.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do compile { target c++14 } } + +struct a { + void b() {} + void c(void (*) () = [] { + if (a a(int auto) {}) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct d { + void e() {} + void f(void (*) () = [] { + for (;d d(int auto) {};) // { dg-error "two or more data types|function type" } + ; + }) {} +}; + +struct g { + void h() {} + void i(void (*) () = [] { + while (g g(int auto) {}) // { dg-error "two or more data types|function type" } + ; + }) {} +}; Index: testsuite/g++.old-deja/g++.jason/cond.C =================================================================== --- testsuite/g++.old-deja/g++.jason/cond.C (revision 260308) +++ testsuite/g++.old-deja/g++.jason/cond.C (working copy) @@ -47,8 +47,7 @@ int main() if (struct B * foo = new B) ; - if (int f () = 1) // { dg-warning "extern" "extern" } - // { dg-error "is initialized like a variable" "var" { target *-*-* } .-1 } + if (int f () = 1) // { dg-error "function type" } ; if (int a[2] = {1, 2}) // { dg-error "extended init" "" { target { ! c++11 } } }