Re: [C++ Patch] PR 63985
OK, thanks. Jason
Re: [C++ Patch] PR 63985
Hi, On 12/23/2014 07:13 PM, Jason Merrill wrote: On 12/23/2014 11:30 AM, Paolo Carlini wrote: if (maybe_range_for_decl) -*maybe_range_for_decl = error_mark_node; +{ + *maybe_range_for_decl = error_mark_node; + if (*equal_loc == UNKNOWN_LOCATION) +tmp_equal_loc = token->location; Even though the range-for case is the only place we care about the initializer location, I'd rather set *equal_loc whenever equal_loc is non-null. We can also use the local initializer location in place of initializer_start_token->location. And let's call it init_loc rather than equal_loc. Ok. I think the below is closer, the only detail I want to mention is that it still checks *init_loc == UNKNOWN_LOCATION before assigning, because the location of the error doesn't change if more than one initializer is present. Likewise for the comma. Thanks, Paolo. /// Index: cp/parser.c === --- cp/parser.c (revision 219042) +++ cp/parser.c (working copy) @@ -2124,7 +2124,8 @@ static tree cp_parser_decltype /* Declarators [gram.dcl.decl] */ static tree cp_parser_init_declarator - (cp_parser *, cp_decl_specifier_seq *, vec *, bool, bool, int, bool *, tree *); + (cp_parser *, cp_decl_specifier_seq *, vec *, + bool, bool, int, bool *, tree *, location_t *); static cp_declarator *cp_parser_declarator (cp_parser *, cp_parser_declarator_kind, int *, bool *, bool, bool); static cp_declarator *cp_parser_direct_declarator @@ -11454,6 +11455,8 @@ cp_parser_simple_declaration (cp_parser* parser, cp_decl_specifier_seq decl_specifiers; int declares_class_or_enum; bool saw_declarator; + location_t comma_loc = UNKNOWN_LOCATION; + location_t init_loc = UNKNOWN_LOCATION; if (maybe_range_for_decl) *maybe_range_for_decl = NULL_TREE; @@ -11528,12 +11531,16 @@ cp_parser_simple_declaration (cp_parser* parser, if (saw_declarator) { - /* If we are processing next declarator, coma is expected */ + /* If we are processing next declarator, comma is expected */ token = cp_lexer_peek_token (parser->lexer); gcc_assert (token->type == CPP_COMMA); cp_lexer_consume_token (parser->lexer); if (maybe_range_for_decl) - *maybe_range_for_decl = error_mark_node; + { + *maybe_range_for_decl = error_mark_node; + if (comma_loc == UNKNOWN_LOCATION) + comma_loc = token->location; + } } else saw_declarator = true; @@ -11545,7 +11552,8 @@ cp_parser_simple_declaration (cp_parser* parser, /*member_p=*/false, declares_class_or_enum, &function_definition_p, - maybe_range_for_decl); + maybe_range_for_decl, + &init_loc); /* If an error occurred while parsing tentatively, exit quickly. (That usually happens when in the body of a function; each statement is treated as a declaration-statement until proven @@ -11631,7 +11639,15 @@ cp_parser_simple_declaration (cp_parser* parser, /* Consume the `;'. */ if (!maybe_range_for_decl) - cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); +cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + else if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) +{ + if (init_loc != UNKNOWN_LOCATION) + error_at (init_loc, "initializer in range-based % loop"); + if (comma_loc != UNKNOWN_LOCATION) + error_at (comma_loc, + "multiple declarations in range-based % loop"); +} done: pop_deferring_access_checks (); @@ -16842,8 +16858,13 @@ cp_parser_asm_definition (cp_parser* parser) parsed declaration if it is an uninitialized single declarator not followed by a `;', or to error_mark_node otherwise. Either way, the trailing `;', if present, will not be consumed. If returned, this declarator will be - created with SD_INITIALIZED but will not call cp_finish_decl. */ + created with SD_INITIALIZED but will not call cp_finish_decl. + If INIT_LOC is not NULL, and *INIT_LOC is equal to UNKNOWN_LOCATION, + and there is an initializer, the pointed location_t is set to the + location of the '=' (or `(', or '{' in C++11) token introducing the + initializer. */ + static tree cp_parser_init_declarator (cp_parser* parser, cp_decl_specifier_seq *decl_specifiers, @@ -16852,7 +16873,8 @@ cp_parser_init_declarator (cp_parser* parser, bool member_p, int declares_class_or_enum, bool* function_definition_p, - tree* maybe_range_for_decl) + tree* may
Re: [C++ Patch] PR 63985
On 12/23/2014 11:30 AM, Paolo Carlini wrote: if (maybe_range_for_decl) - *maybe_range_for_decl = error_mark_node; + { + *maybe_range_for_decl = error_mark_node; + if (*equal_loc == UNKNOWN_LOCATION) + tmp_equal_loc = token->location; Even though the range-for case is the only place we care about the initializer location, I'd rather set *equal_loc whenever equal_loc is non-null. We can also use the local initializer location in place of initializer_start_token->location. And let's call it init_loc rather than equal_loc. Jason
Re: [C++ Patch] PR 63985
Hi again, thus, in order to deal with the various subissues we have got, I prepared the below, which passes testing. As you can see, the diagnostic triggers only once, for the left-most '=' and/or the left-most ','. I suppose that's Ok, I'm not 100% sure about the wording of the error messages though. Otherwise, I'm quite happy with the patch ;) modulo maybe the need to pass around a location_t*, isn't something we do very often. Let me know... Thanks, Paolo. // Index: cp/parser.c === --- cp/parser.c (revision 219042) +++ cp/parser.c (working copy) @@ -2124,7 +2124,8 @@ static tree cp_parser_decltype /* Declarators [gram.dcl.decl] */ static tree cp_parser_init_declarator - (cp_parser *, cp_decl_specifier_seq *, vec *, bool, bool, int, bool *, tree *); + (cp_parser *, cp_decl_specifier_seq *, vec *, + bool, bool, int, bool *, tree *, location_t *); static cp_declarator *cp_parser_declarator (cp_parser *, cp_parser_declarator_kind, int *, bool *, bool, bool); static cp_declarator *cp_parser_direct_declarator @@ -11454,6 +11455,8 @@ cp_parser_simple_declaration (cp_parser* parser, cp_decl_specifier_seq decl_specifiers; int declares_class_or_enum; bool saw_declarator; + location_t comma_loc = UNKNOWN_LOCATION; + location_t equal_loc = UNKNOWN_LOCATION; if (maybe_range_for_decl) *maybe_range_for_decl = NULL_TREE; @@ -11528,12 +11531,16 @@ cp_parser_simple_declaration (cp_parser* parser, if (saw_declarator) { - /* If we are processing next declarator, coma is expected */ + /* If we are processing next declarator, comma is expected */ token = cp_lexer_peek_token (parser->lexer); gcc_assert (token->type == CPP_COMMA); cp_lexer_consume_token (parser->lexer); if (maybe_range_for_decl) - *maybe_range_for_decl = error_mark_node; + { + *maybe_range_for_decl = error_mark_node; + if (comma_loc == UNKNOWN_LOCATION) + comma_loc = token->location; + } } else saw_declarator = true; @@ -11545,7 +11552,8 @@ cp_parser_simple_declaration (cp_parser* parser, /*member_p=*/false, declares_class_or_enum, &function_definition_p, - maybe_range_for_decl); + maybe_range_for_decl, + &equal_loc); /* If an error occurred while parsing tentatively, exit quickly. (That usually happens when in the body of a function; each statement is treated as a declaration-statement until proven @@ -11631,7 +11639,15 @@ cp_parser_simple_declaration (cp_parser* parser, /* Consume the `;'. */ if (!maybe_range_for_decl) - cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); +cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + else if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) +{ + if (equal_loc != UNKNOWN_LOCATION) + error_at (equal_loc, "initializer in range-based % loop"); + if (comma_loc != UNKNOWN_LOCATION) + error_at (comma_loc, + "multiple declarations in range-based % loop"); +} done: pop_deferring_access_checks (); @@ -16842,8 +16858,13 @@ cp_parser_asm_definition (cp_parser* parser) parsed declaration if it is an uninitialized single declarator not followed by a `;', or to error_mark_node otherwise. Either way, the trailing `;', if present, will not be consumed. If returned, this declarator will be - created with SD_INITIALIZED but will not call cp_finish_decl. */ + created with SD_INITIALIZED but will not call cp_finish_decl. + If MAYBE_RANGE_FOR_DECL is not NULL, and *EQUAL_LOC is equal to + UNKNOWN_LOCATION, and there is an initializer, the pointed location_t + is set to the location of the '=' (or `(', or '{' in C++11) token + introducing the initializer. */ + static tree cp_parser_init_declarator (cp_parser* parser, cp_decl_specifier_seq *decl_specifiers, @@ -16852,7 +16873,8 @@ cp_parser_init_declarator (cp_parser* parser, bool member_p, int declares_class_or_enum, bool* function_definition_p, - tree* maybe_range_for_decl) + tree* maybe_range_for_decl, + location_t* equal_loc) { cp_token *token = NULL, *asm_spec_start_token = NULL, *attributes_start_token = NULL; @@ -16875,6 +16897,7 @@ cp_parser_init_declarator (cp_parser* parser, tree pushed_scope = NULL_TREE; bool range_for_decl_p = false; bool saved_default_arg_ok_p = parser->default_arg_ok_p; + location_t tmp_equal_lo
Re: [C++ Patch] PR 63985
.. and, to make things more interesting ;) for: for (int a, b, c: arr) we currently give: 63985.C:7:19: error: expected initializer before β:β token 63985.C:7:21: warning: range-based βforβ loops only available with -std=c++11 or -std=gnu++11 because, inside cp_parse_init_declarator we are in "error mode" for range-based after the first comma thus, as a for loop, we complain about the missing initializer; then in cp_parser_for_init_statement we notice the ':' and we give the warning / we think is a range-based. Paolo.
Re: [C++ Patch] PR 63985
On 12/22/2014 04:51 PM, Paolo Carlini wrote: Well, if they are both right, could we maybe do something closer to clang, instead, thus not consider the for loop a range-based for loop only because after the initializer there is a colon when the decl is error_mark_node? I think we want to tell the user that you can't have an explicit initializer in a range-based for loop, since that's the mistake that they've made; that seems more helpful than saying you can't have a colon after a for-init-statement. Jason
Re: [C++ Patch] PR 63985
Hi, On 12/22/2014 10:12 PM, Jason Merrill wrote: We could also peek for a colon in cp_parser_init_declarator after parsing the initializer, and give an error then. I see. For the record, clang vs edg appear to handle this case differently: clang considers it a wrong for loop, edg a wrong range-based for loop. Humm... They're both right. :) Well, if they are both right, could we maybe do something closer to clang, instead, thus not consider the for loop a range-based for loop only because after the initializer there is a colon when the decl is error_mark_node? As an heuristics I think it may make sense in these borderline cases because the declarator is more restricted for a range-based. The below passes testing. Thanks, Paolo. / Index: cp/parser.c === --- cp/parser.c (revision 219030) +++ cp/parser.c (working copy) @@ -10894,16 +10894,26 @@ cp_parser_for_init_statement (cp_parser* parser, t parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p; if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) { - /* It is a range-for, consume the ':' */ - cp_lexer_consume_token (parser->lexer); - is_range_for = true; - if (cxx_dialect < cxx11) + if (*decl == error_mark_node) { - pedwarn (cp_lexer_peek_token (parser->lexer)->location, 0, - "range-based % loops only available with " - "-std=c++11 or -std=gnu++11"); - *decl = error_mark_node; + /* Give an error and consume the ':' anyway for better +error recovery. */ + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + cp_lexer_consume_token (parser->lexer); } + else + { + /* It is a range-for, consume the ':' */ + cp_lexer_consume_token (parser->lexer); + is_range_for = true; + if (cxx_dialect < cxx11) + { + pedwarn (cp_lexer_peek_token (parser->lexer)->location, 0, + "range-based % loops only available with " + "-std=c++11 or -std=gnu++11"); + *decl = error_mark_node; + } + } } else /* The ';' is not consumed yet because we told Index: testsuite/g++.dg/cpp0x/range-for29.C === --- testsuite/g++.dg/cpp0x/range-for29.C(revision 0) +++ testsuite/g++.dg/cpp0x/range-for29.C(working copy) @@ -0,0 +1,10 @@ +// PR c++/63985 +// { dg-require-effective-target c++11 } + +int main() +{ + int arr; + for (int i = 5: arr) // { dg-error "expected ';'" } +return 1; + return 0; +}
Re: [C++ Patch] PR 63985
On 12/22/2014 03:11 PM, Paolo Carlini wrote: That would be the place. But, at that point, it could still be a normal for loop, thus we can't just give an error. Ah, yes. Assigning error_mark_node on the other hand is correct, because later, if we find the colon, we realize that the declaration is wrong because has an initializer. We could also peek for a colon in cp_parser_init_declarator after parsing the initializer, and give an error then. For the record, clang vs edg appear to handle this case differently: clang considers it a wrong for loop, edg a wrong range-based for loop. Humm... They're both right. :) Jason
Re: [C++ Patch] PR 63985
Hi, On 12/22/2014 07:55 PM, Jason Merrill wrote: On 12/04/2014 07:51 AM, Paolo Carlini wrote: Ideally, it would be nice to say something more detailed about the invalid declaration, but that doesn't seem trivial... How about giving that error in cp_parser_init_declarator? /* An `=' or an `(', or an '{' in C++0x, indicates an initializer. */ if (token->type == CPP_EQ || token->type == CPP_OPEN_PAREN || token->type == CPP_OPEN_BRACE) { is_initialized = SD_INITIALIZED; initialization_kind = token->type; if (maybe_range_for_decl) *maybe_range_for_decl = error_mark_node; Here. That would be the place. But, at that point, it could still be a normal for loop, thus we can't just give an error. Assigning error_mark_node on the other hand is correct, because later, if we find the colon, we realize that the declaration is wrong because has an initializer. For the record, clang vs edg appear to handle this case differently: clang considers it a wrong for loop, edg a wrong range-based for loop. Humm... Paolo.
Re: [C++ Patch] PR 63985
On 12/04/2014 07:51 AM, Paolo Carlini wrote: Ideally, it would be nice to say something more detailed about the invalid declaration, but that doesn't seem trivial... How about giving that error in cp_parser_init_declarator? /* An `=' or an `(', or an '{' in C++0x, indicates an initializer. */ if (token->type == CPP_EQ || token->type == CPP_OPEN_PAREN || token->type == CPP_OPEN_BRACE) { is_initialized = SD_INITIALIZED; initialization_kind = token->type; if (maybe_range_for_decl) *maybe_range_for_decl = error_mark_node; Here. Jason
[Ping] Re: [C++ Patch] PR 63985
Hi, On 12/04/2014 01:54 PM, Paolo Carlini wrote: ... oops, sent the wrong patch. See the below instead. Paolo. It occurs to me that a while ago I sent this patch: what do you think? Is the diagnostic good enough? https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00376.html https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00378.html Thanks, Paolo.
Re: [C++ Patch] PR 63985
... oops, sent the wrong patch. See the below instead. Paolo. /// Index: cp/parser.c === --- cp/parser.c (revision 218348) +++ cp/parser.c (working copy) @@ -10841,6 +10841,7 @@ cp_parser_for_init_statement (cp_parser* parser, t { bool is_range_for = false; bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p; + location_t loc = cp_lexer_peek_token (parser->lexer)->location; if (cp_lexer_next_token_is (parser->lexer, CPP_NAME) && cp_lexer_nth_token_is (parser->lexer, 2, CPP_COLON)) @@ -10881,6 +10882,8 @@ cp_parser_for_init_statement (cp_parser* parser, t "-std=c++11 or -std=gnu++11"); *decl = error_mark_node; } + else if (*decl == error_mark_node) + error_at (loc, "invalid declaration in range-based % loop"); } else /* The ';' is not consumed yet because we told Index: testsuite/g++.dg/cpp0x/range-for29.C === --- testsuite/g++.dg/cpp0x/range-for29.C(revision 0) +++ testsuite/g++.dg/cpp0x/range-for29.C(working copy) @@ -0,0 +1,10 @@ +// PR c++/63985 +// { dg-require-effective-target c++11 } + +int main() +{ + int arr; + for (int i = 5: arr) // { dg-error "invalid declaration" } +return 1; + return 0; +}