public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [C++ Patch] PR 63985
@ 2014-12-04 12:51 Paolo Carlini
  2014-12-04 12:54 ` Paolo Carlini
  2014-12-22 18:57 ` Jason Merrill
  0 siblings, 2 replies; 13+ messages in thread
From: Paolo Carlini @ 2014-12-04 12:51 UTC (permalink / raw)
  To: gcc-patches; +Cc: Jason Merrill

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

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.

/////////////////////////

[-- Attachment #2: CL_63985 --]
[-- Type: text/plain, Size: 289 bytes --]

/cp
2014-12-04  Paolo Carlini  <paolo.carlini@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.carlini@oracle.com>

	PR c++/63985
	* g++.dg/cpp0x/range-for29.C: New.

[-- Attachment #3: patch_63985 --]
[-- Type: text/plain, Size: 1299 bytes --]

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;
+}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-04 12:51 [C++ Patch] PR 63985 Paolo Carlini
@ 2014-12-04 12:54 ` Paolo Carlini
  2014-12-22 16:38   ` [Ping] " Paolo Carlini
  2014-12-22 18:57 ` Jason Merrill
  1 sibling, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-04 12:54 UTC (permalink / raw)
  To: gcc-patches; +Cc: Jason Merrill

[-- Attachment #1: Type: text/plain, Size: 84 bytes --]

... oops, sent the wrong patch. See the below instead.

Paolo.

///////////////////

[-- Attachment #2: patch_63985_2 --]
[-- Type: text/plain, Size: 1304 bytes --]

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;
+}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Ping] Re: [C++ Patch] PR 63985
  2014-12-04 12:54 ` Paolo Carlini
@ 2014-12-22 16:38   ` Paolo Carlini
  0 siblings, 0 replies; 13+ messages in thread
From: Paolo Carlini @ 2014-12-22 16:38 UTC (permalink / raw)
  To: gcc-patches; +Cc: Jason Merrill

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-04 12:51 [C++ Patch] PR 63985 Paolo Carlini
  2014-12-04 12:54 ` Paolo Carlini
@ 2014-12-22 18:57 ` Jason Merrill
  2014-12-22 20:26   ` Paolo Carlini
  1 sibling, 1 reply; 13+ messages in thread
From: Jason Merrill @ 2014-12-22 18:57 UTC (permalink / raw)
  To: Paolo Carlini, gcc-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-22 18:57 ` Jason Merrill
@ 2014-12-22 20:26   ` Paolo Carlini
  2014-12-22 21:14     ` Jason Merrill
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-22 20:26 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-22 20:26   ` Paolo Carlini
@ 2014-12-22 21:14     ` Jason Merrill
  2014-12-22 21:58       ` Paolo Carlini
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Merrill @ 2014-12-22 21:14 UTC (permalink / raw)
  To: Paolo Carlini, gcc-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-22 21:14     ` Jason Merrill
@ 2014-12-22 21:58       ` Paolo Carlini
  2014-12-22 22:56         ` Jason Merrill
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-22 21:58 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

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.

/////////////

[-- Attachment #2: patch_63985_3 --]
[-- Type: text/plain, Size: 1827 bytes --]

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;
+}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-22 21:58       ` Paolo Carlini
@ 2014-12-22 22:56         ` Jason Merrill
  2014-12-23 16:31           ` Paolo Carlini
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Merrill @ 2014-12-22 22:56 UTC (permalink / raw)
  To: Paolo Carlini, gcc-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-22 22:56         ` Jason Merrill
@ 2014-12-23 16:31           ` Paolo Carlini
  2014-12-23 16:46             ` Paolo Carlini
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-23 16:31 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches

.. 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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-23 16:31           ` Paolo Carlini
@ 2014-12-23 16:46             ` Paolo Carlini
  2014-12-23 18:39               ` Jason Merrill
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-23 16:46 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

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.

//////////////////////////

[-- Attachment #2: patch_63985_4 --]
[-- Type: text/plain, Size: 5975 bytes --]

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<deferred_access_check, va_gc> *, bool, bool, int, bool *, tree *);
+  (cp_parser *, cp_decl_specifier_seq *, vec<deferred_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 = parser->default_arg_ok_p;
+  location_t tmp_equal_loc = UNKNOWN_LOCATION;
 
   /* Gather the attributes that were provided with the
      decl-specifiers.  */
@@ -17040,7 +17063,11 @@ cp_parser_init_declarator (cp_parser* parser,
       is_initialized = SD_INITIALIZED;
       initialization_kind = token->type;
       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;
+	}
 
       if (token->type == CPP_EQ
 	  && function_declarator_p (declarator))
@@ -17063,7 +17090,8 @@ cp_parser_init_declarator (cp_parser* parser,
 	    range_for_decl_p = true;
 	  else
 	    {
-	      cp_parser_error (parser, "expected initializer");
+	      if (!maybe_range_for_decl)
+		cp_parser_error (parser, "expected initializer");
 	      return error_mark_node;
 	    }
 	}
@@ -17168,6 +17196,8 @@ cp_parser_init_declarator (cp_parser* parser,
 	    finish_lambda_scope ();
 	  if (initializer == error_mark_node)
 	    cp_parser_skip_to_end_of_statement (parser);
+	  else if (initializer && tmp_equal_loc != UNKNOWN_LOCATION)
+	    *equal_loc = tmp_equal_loc;
 	}
     }
 
@@ -23726,7 +23756,7 @@ cp_parser_single_declaration (cp_parser* parser,
 				        member_p,
 				        declares_class_or_enum,
 				        &function_definition_p,
-					NULL);
+					NULL, NULL);
 
     /* 7.1.1-1 [dcl.stc]
 
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,13 @@
+// PR c++/63985
+// { dg-require-effective-target c++11 }
+
+void foo()
+{
+  int arr;
+
+  for (int i = 5: arr)  // { dg-error "initializer in range-based" }
+    ;
+
+  for (int i, j: arr)   // { dg-error "multiple declarations in range-based" }
+    ;
+}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-23 16:46             ` Paolo Carlini
@ 2014-12-23 18:39               ` Jason Merrill
  2014-12-23 21:02                 ` Paolo Carlini
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Merrill @ 2014-12-23 18:39 UTC (permalink / raw)
  To: Paolo Carlini, gcc-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-23 18:39               ` Jason Merrill
@ 2014-12-23 21:02                 ` Paolo Carlini
  2014-12-23 22:24                   ` Jason Merrill
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Carlini @ 2014-12-23 21:02 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

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.

///////////////////////

[-- Attachment #2: patch_63985_5 --]
[-- Type: text/plain, Size: 6390 bytes --]

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<deferred_access_check, va_gc> *, bool, bool, int, bool *, tree *);
+  (cp_parser *, cp_decl_specifier_seq *, vec<deferred_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* maybe_range_for_decl)
+			   tree* maybe_range_for_decl,
+			   location_t* init_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_init_loc = UNKNOWN_LOCATION;
 
   /* Gather the attributes that were provided with the
      decl-specifiers.  */
@@ -17041,6 +17064,9 @@ cp_parser_init_declarator (cp_parser* parser,
       initialization_kind = token->type;
       if (maybe_range_for_decl)
 	*maybe_range_for_decl = error_mark_node;
+      tmp_init_loc = token->location;
+      if (init_loc && *init_loc == UNKNOWN_LOCATION)
+	*init_loc = tmp_init_loc;
 
       if (token->type == CPP_EQ
 	  && function_declarator_p (declarator))
@@ -17063,7 +17089,8 @@ cp_parser_init_declarator (cp_parser* parser,
 	    range_for_decl_p = true;
 	  else
 	    {
-	      cp_parser_error (parser, "expected initializer");
+	      if (!maybe_range_for_decl)
+		cp_parser_error (parser, "expected initializer");
 	      return error_mark_node;
 	    }
 	}
@@ -17135,7 +17162,6 @@ cp_parser_init_declarator (cp_parser* parser,
     {
       if (function_declarator_p (declarator))
 	{
-	  cp_token *initializer_start_token = cp_lexer_peek_token (parser->lexer);
 	   if (initialization_kind == CPP_EQ)
 	     initializer = cp_parser_pure_specifier (parser);
 	   else
@@ -17144,8 +17170,7 @@ cp_parser_init_declarator (cp_parser* parser,
 		  know what the user intended, so just silently
 		  consume the initializer.  */
 	       if (decl != error_mark_node)
-		 error_at (initializer_start_token->location,
-			   "initializer provided for function");
+		 error_at (tmp_init_loc, "initializer provided for function");
 	       cp_parser_skip_to_closing_parenthesis (parser,
 						      /*recovering=*/true,
 						      /*or_comma=*/false,
@@ -23726,7 +23751,7 @@ cp_parser_single_declaration (cp_parser* parser,
 				        member_p,
 				        declares_class_or_enum,
 				        &function_definition_p,
-					NULL);
+					NULL, NULL);
 
     /* 7.1.1-1 [dcl.stc]
 
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,13 @@
+// PR c++/63985
+// { dg-require-effective-target c++11 }
+
+void foo()
+{
+  int arr;
+
+  for (int i = 5: arr)  // { dg-error "initializer in range-based" }
+    ;
+
+  for (int i, j: arr)   // { dg-error "multiple declarations in range-based" }
+    ;
+}

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [C++ Patch] PR 63985
  2014-12-23 21:02                 ` Paolo Carlini
@ 2014-12-23 22:24                   ` Jason Merrill
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Merrill @ 2014-12-23 22:24 UTC (permalink / raw)
  To: Paolo Carlini, gcc-patches

OK, thanks.

Jason

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-12-23 21:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-04 12:51 [C++ Patch] PR 63985 Paolo Carlini
2014-12-04 12:54 ` Paolo Carlini
2014-12-22 16:38   ` [Ping] " Paolo Carlini
2014-12-22 18:57 ` Jason Merrill
2014-12-22 20:26   ` Paolo Carlini
2014-12-22 21:14     ` Jason Merrill
2014-12-22 21:58       ` Paolo Carlini
2014-12-22 22:56         ` Jason Merrill
2014-12-23 16:31           ` Paolo Carlini
2014-12-23 16:46             ` Paolo Carlini
2014-12-23 18:39               ` Jason Merrill
2014-12-23 21:02                 ` Paolo Carlini
2014-12-23 22:24                   ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).