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
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 *, vecdeferred_access_check, va_gc *,
bool, bool, int, bool *, tree *);
+ (cp_parser *, cp_decl_specifier_seq *, vecdeferred_access_check, va_gc *,
+ 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 %for% loop);
+ if (comma_loc != UNKNOWN_LOCATION)
+ error_at (comma_loc,
+ multiple declarations in range-based %for% 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
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,
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 *, vecdeferred_access_check, va_gc *,
bool, bool, int, bool *, tree *);
+ (cp_parser *, cp_decl_specifier_seq *, vecdeferred_access_check, va_gc *,
+ 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 %for% loop);
+ if (comma_loc != UNKNOWN_LOCATION)
+ error_at (comma_loc,
+ multiple declarations in range-based %for% 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*
Re: [C++ Patch] PR 63985
OK, thanks. 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
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
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/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 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 %for% 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 %for% 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 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
[C++ Patch] PR 63985
Hi,
this accepts-invalid is about an invalid loop of the form:
for (int i = 5: arr)
thus it starts with an initialized declaration, which would be legal in
a normal for loop, but then the colon means that it can only be an
invalid range-based for loop. Ideally, it would be nice to say something
more detailed about the invalid declaration, but that doesn't seem
trivial... Would something like the below be ok for now? Tested
x86_64-linux.
Thanks,
Paolo.
/
/cp
2014-12-04 Paolo Carlini paolo.carl...@oracle.com
PR c++/63985
* parser.c (cp_parser_for_init_statement): Reject invalid declarations
in range-based for loops.
/testsuite
2014-12-04 Paolo Carlini paolo.carl...@oracle.com
PR c++/63985
* g++.dg/cpp0x/range-for29.C: New.
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;
}
+ if (*decl == error_mark_node)
+ error_at (loc, invalid declaration in range-based %for% 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;
+}
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 %for% 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;
+}
