RE: Range-based for in c++98

RE: Range-based for in c++98

[Rodrigo Rivas]

> This is quite unreadable and not very informative.
Totally agree.

> Here there are two problems...
> snipped

I think that you are taking the wrong approach: you call
"cp_parser_range_for" with C++98 and then if such a loop is parsed
(the ':') you issue an error.
Maybe you should try to add the check to the "cp_parser_c_for" function.
Note that a range-based for always have a declaration in the first
sub-statement, so next the "cp_parser_c_for" will expect a ';' or a
initialization ('(', '=' or '{', IIRC).
If instead of that, a ':' is found there is your chance to diagnose!

Regards.
Rodrigo

RE: Range-based for in c++98

[Magnus Fromreide]

On Mon, 2010-09-20 at 10:39 +0200, Rodrigo Rivas wrote:
> > This is quite unreadable and not very informative.
> Totally agree.
> 
> > Here there are two problems...
> > snipped
> 
> I think that you are taking the wrong approach: you call
> "cp_parser_range_for" with C++98 and then if such a loop is parsed
> (the ':') you issue an error.
> Maybe you should try to add the check to the "cp_parser_c_for" function.
> Note that a range-based for always have a declaration in the first
> sub-statement, so next the "cp_parser_c_for" will expect a ';' or a
> initialization ('(', '=' or '{', IIRC).
> If instead of that, a ':' is found there is your chance to diagnose!

Well, yes, this is true but there is still the issue that

void f() {
for(class C{};;)
  ;
}

generates the message

error: types may not be defined in range-based for loops

when compiled with -std=c++0x and no patches and that is odd since this
loop isn't range-based.

/MF

RE: Range-based for in c++98

[Rodrigo]

> Well, yes, this is true but there is still the issue that
> 
> void f() {
> for(class C{};;)
>   ;
> }
> 
> generates the message
> 
> error: types may not be defined in range-based for loops
> 
> when compiled with -std=c++0x and no patches and that is odd since this
> loop isn't range-based.
Oh, I see...
The problem is that regular C for loops allows defining types in the
first statement, whereas range-based for loops do not. That, if I'm
reading the draft correctly.

So the problem is that when setting the
type_definition_forbidden_message to non-NULL, when it sees the type it
calls error() instead of cp_parser_error() so the
cp_parser_parse_tentatively has no effect.

The easy solution would be to remove the assignment to
type_definition_forbidden_message and then check for this case
particulary.
Or we can change the cp_parser_check_type_definition() function to call
cp_parser_error() instead of error().

Or we can just accept the type definition in the range-for and hope that
nobody will notice :P

Regards.
Rodrigo

Re: Range-based for in c++98

[Rodrigo Rivas]

On second reading of the C++0x draft in cannot find any reason of what
the new type in range-fors should not be allowed.

Maybe someone can read it differently?

Regard.
Rodrigo

Re: Range-based for in c++98

[Manuel López-Ibáñez]

On 20 September 2010 20:19, Rodrigo <rodrigorivascosta@gmail.com> wrote:
>> Well, yes, this is true but there is still the issue that
>>
>> void f() {
>> for(class C{};;)
>>   ;
>> }
>>
>> generates the message
>>
>> error: types may not be defined in range-based for loops
>>
>> when compiled with -std=c++0x and no patches and that is odd since this
>> loop isn't range-based.
> Oh, I see...
> The problem is that regular C for loops allows defining types in the
> first statement, whereas range-based for loops do not. That, if I'm
> reading the draft correctly.
>
> So the problem is that when setting the
> type_definition_forbidden_message to non-NULL, when it sees the type it
> calls error() instead of cp_parser_error() so the
> cp_parser_parse_tentatively has no effect.
>
> The easy solution would be to remove the assignment to
> type_definition_forbidden_message and then check for this case
> particulary.

cp_parser_type_specifier_seq could return some indication of why the
parsing has failed or whether it has parsed a declaration. This is
much more useful than just throwing an error. In general this is true
of the whole C++ parser.

> Or we can change the cp_parser_check_type_definition() function to call
> cp_parser_error() instead of error().

No please, cp_parser_error() is almost always annoying and
uninformative saying things like:

expected __asm__ before ';'

because it lacks context.

> Or we can just accept the type definition in the range-for and hope that
> nobody will notice :P

You will have to diagnose it with -pedantic anyway.

>
> Regards.
> Rodrigo
>
>

Re: Range-based for in c++98

[Rodrigo Rivas]

>> The easy solution would be to remove the assignment to
>> type_definition_forbidden_message and then check for this case
>> particulary.
>
> cp_parser_type_specifier_seq could return some indication of why the
> parsing has failed or whether it has parsed a declaration. This is
> much more useful than just throwing an error. In general this is true
> of the whole C++ parser.
You are right. Being a C++ parser it would be appropriate to implement
some kind of exception-like error management.
But this is way out of my scope (pun intended).

>> Or we can change the cp_parser_check_type_definition() function to call
>> cp_parser_error() instead of error().
>
> No please, cp_parser_error() is almost always annoying and
> uninformative saying things like:
>
> expected __asm__ before ';'
>
> because it lacks context.
I said that just because of c_parser_error respect the tentative flag.
I think that you can put whatever message you like, can't you?

>
>> Or we can just accept the type definition in the range-for and hope that
>> nobody will notice :P
>
> You will have to diagnose it with -pedantic anyway.
Are you sure? As I said in other post, I am no longer sure that the
C++0x draft forbids the type definition in this context.
But I'm no expert in standarese, so I'm still undecided.

Re: Range-based for in c++98

[Manuel López-Ibáñez]

On 21 September 2010 01:06, Rodrigo Rivas <rodrigorivascosta@gmail.com> wrote:
>>> The easy solution would be to remove the assignment to
>>> type_definition_forbidden_message and then check for this case
>>> particulary.
>>
>> cp_parser_type_specifier_seq could return some indication of why the
>> parsing has failed or whether it has parsed a declaration. This is
>> much more useful than just throwing an error. In general this is true
>> of the whole C++ parser.
> You are right. Being a C++ parser it would be appropriate to implement
> some kind of exception-like error management.

You do not need exceptions to implement what I said, just to return a
value. A boolean would suffice to detect whether you parsed a
definition.

> But this is way out of my scope (pun intended).
>
>>> Or we can change the cp_parser_check_type_definition() function to call
>>> cp_parser_error() instead of error().
>>
>> No please, cp_parser_error() is almost always annoying and
>> uninformative saying things like:
>>
>> expected __asm__ before ';'
>>
>> because it lacks context.
> I said that just because of c_parser_error respect the tentative flag.
> I think that you can put whatever message you like, can't you?

The "before whatever" is hardcoded. And tentative parsing is
thoroughly abused in the parser. It means that you can get an error
but you do not really know what happened, so you ignore it and try
something else, which is bad in many (most?) cases. Most fixes I have
done to the C++ parser error-handling involved removing tentative
parsing.

Cheers,

Manuel.

Re: Range-based for in c++98

[Rodrigo Rivas]

> You do not need exceptions to implement what I said, just to return a
> value. A boolean would suffice to detect whether you parsed a
> definition.
Sure, I was talking generally.
> The "before whatever" is hardcoded. And tentative parsing is
> thoroughly abused in the parser.
Well, I tried to parse it normally, but the function called from the
regular for loop expected after the declaration one of the following
';', ',', '(', '=', '{' if I remember right. When it found a ':' it
choked and emitted an error. Probably it would be better to let the
unknown char to be handled by the calling function. In this way if I
see a ':' I know that it is a range-based for and no tentative is
needed.

But this is too big a change for me at the moment. For now, in order
to solve Magnus's problem I suggest to just remove the check for new
type in cp_parser_range_for().

Regards.
Rodrigo

Re: Range-based for in c++98

[Jason Merrill]

On 09/20/2010 07:06 PM, Rodrigo Rivas wrote:
> Are you sure? As I said in other post, I am no longer sure that the
> C++0x draft forbids the type definition in this context.
> But I'm no expert in standarese, so I'm still undecided.

It took me some searching, but yes, it does:

"A type-specifier-seq shall not define a class or enumeration unless it 
appears in the type-id of an alias-declaration (7.1.3)."

Normal declarations don't have a type-specifier-seq, they have a 
decl-specifier-seq.

I would change cp_parser_range_for to use cp_parser_decl_specifier_seq 
instead of cp_parser_type_specifier_seq and then wait to complain about 
defining a type until after we've seen the ':'.

Jason

Re: Range-based for in c++98

[Rodrigo Rivas]

2010/10/1 Jason Merrill <jason@redhat.com>:
> It took me some searching, but yes, it does:
>
> "A type-specifier-seq shall not define a class or enumeration unless it
> appears in the type-id of an alias-declaration (7.1.3)."
>
> Normal declarations don't have a type-specifier-seq, they have a
> decl-specifier-seq.
No wonder I couldn't find it!

> I would change cp_parser_range_for to use cp_parser_decl_specifier_seq
> instead of cp_parser_type_specifier_seq and then wait to complain about
> defining a type until after we've seen the ':'.
I already tried that, but it didn't work. It seemed to me that it was
because it called cp_parser_commit_to_tentative_parse(), and if then I
wanted to roll-back the parsing of the range-for, I went badly. I had
to agree with Manuel that the tentative parsing is a bit messy...

But, I managed to solve this particual issue with the following patch
(no improvement in the error messages, however):

Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 164871)
+++ gcc/cp/parser.c	(working copy)
@@ -8644,21 +8644,13 @@ cp_parser_range_for (cp_parser *parser)
   tree stmt, range_decl, range_expr;
   cp_decl_specifier_seq type_specifiers;
   cp_declarator *declarator;
-  const char *saved_message;
   tree attributes, pushed_scope;

   cp_parser_parse_tentatively (parser);
-  /* New types are not allowed in the type-specifier-seq for a
-     range-based for loop.  */
-  saved_message = parser->type_definition_forbidden_message;
-  parser->type_definition_forbidden_message
-    = G_("types may not be defined in range-based for loops");
   /* Parse the type-specifier-seq.  */
   cp_parser_type_specifier_seq (parser, /*is_declaration==*/true,
-				/*is_trailing_return=*/false,
+				/*is_trailing_return=*/true,
 				&type_specifiers);
-  /* 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))
     {

Admittedly, this is not a "trailing_return_type", but AFAICT it has
exactly the same restrictions. Maybe renaming the parameter would be a
good idea.

Regards.
Rodrigo

Re: Range-based for in c++98

[Jason Merrill]

On 10/02/2010 01:50 PM, Rodrigo Rivas wrote:
>> I would change cp_parser_range_for to use cp_parser_decl_specifier_seq
>> > instead of cp_parser_type_specifier_seq and then wait to complain about
>> > defining a type until after we've seen the ':'.
> I already tried that, but it didn't work. It seemed to me that it was
> because it called cp_parser_commit_to_tentative_parse(), and if then I
> wanted to roll-back the parsing of the range-for, I went badly. I had
> to agree with Manuel that the tentative parsing is a bit messy...

Ah, yes.  So we should share the parsing of the decl-specifier-seq with 
the C-style for loop, which allows us to avoid the tentative parsing.

> Admittedly, this is not a "trailing_return_type", but AFAICT it has
> exactly the same restrictions.

The restrictions are slightly different; in the case of a trailing 
return type a class-specifier is not one of the expansions, so we don't 
treat a { as beginning a class body.  In the case of a 
type-specifier-seq, we do treat it as beginning a class body, but we 
give an error about it.

Jason

Re: Range-based for in c++98

[Rodrigo Rivas]

> Ah, yes.  So we should share the parsing of the decl-specifier-seq with the
> C-style for loop, which allows us to avoid the tentative parsing.
That was my original idea, but the C-style loop calls
cp_parser_simple_declaration(), that shouts at the ':'. So we should
either modify it to accept the ':' or split it in two. Both options
are well beyond my intentions.
Anyway the C-style for loop does use tentative parsing, so you don't
avoid it completely.

>> Admittedly, this is not a "trailing_return_type", but AFAICT it has
>> exactly the same restrictions.
>
> The restrictions are slightly different; in the case of a trailing return
> type a class-specifier is not one of the expansions, so we don't treat a {
> as beginning a class body.  In the case of a type-specifier-seq, we do treat
> it as beginning a class body, but we give an error about it.

Well, the net effect is the same: the tentative parsing is aborted.
Then the C-style loop for parses the code, and it is this that prints
the error message if needed.

Regards.
Rodrigo

Re: Range-based for in c++98

[Jason Merrill]

On 10/04/2010 10:59 AM, Rodrigo Rivas wrote:
>>> Admittedly, this is not a "trailing_return_type", but AFAICT it has
>>> exactly the same restrictions.
>>
>> The restrictions are slightly different; in the case of a trailing return
>> type a class-specifier is not one of the expansions, so we don't treat a {
>> as beginning a class body.  In the case of a type-specifier-seq, we do treat
>> it as beginning a class body, but we give an error about it.
>
> Well, the net effect is the same: the tentative parsing is aborted.
> Then the C-style loop for parses the code, and it is this that prints
> the error message if needed.

True, it's just a question of what error you get from something like

for (struct A {} a : vector<A>());

that is, is this parsed as a range-based for loop which invalidly 
defines a type, or as syntax nonsense?  Either way you get 
standard-conforming behavior, but the former gives a more useful diagnostic.

I think the right answer is, as you say, to split up 
cp_parser_simple_declaration so we can share the 
cp_parser_decl_specifier_seq call between the two cases.

Jason

Re: Range-based for in c++98

[Nicola Pero]

>> Ah, yes.  So we should share the parsing of the decl-specifier-seq with the
>> C-style for loop, which allows us to avoid the tentative parsing.
>
> That was my original idea, but the C-style loop calls
> cp_parser_simple_declaration(), that shouts at the ':'. So we should
> either modify it to accept the ':' or split it in two. Both options
> are well beyond my intentions.

I just implemented "fast enumeration" (ie, "for (object in array) { ... }")
for Objective-C, and I was now planning on doing it for Objective-C++ too. ;-)

If you're doing range-based for-loops for C++, we may as well share ideas ;-)

What I've done for Objective-C fast enumeration (and what Apple did before me as well)
is implemented your idea of modifying the declaration parsing to accept ':' (well, 'in'
in the ObjC case) to terminate a declaration in that context:

 * when you start parsing the 'for', a flag is turned on in the parser that means that
we are "maybe in a fast enumeration"

 * then, you go on parsing your usual C for loop, but the code that parses expressions/declarations
has a special case for when it's "maybe in a fast enumeration", in which case it accepts
"in" as terminating the expression/declaration (as well as ";"), and moreover if it finds "in" in
there, it will save the declaration specially and make sure to communicate back to the for
parsing code that we are "really" in a fast enumeration

 * when the initial expression/declaration has been parsed, the for parsing code turns off the 
"maybe in a fast enumeration" flag; it now knows whether it's a normal for loop or a fast enumeration 
one and everything is easy from there on

It sounds like the same could apply to C++ range-based for-loops ?  Except you use ':' instead 
of 'in' ?  We could almost share the code - ie, set a single flag in the C++/ObjC++ parser to say we are
at the beginning of a 'for' loop, and have that flag turn on recognizing ':' (for range-based loops) 
and 'in' (only if (dialect_objc()), for fast ObjC++ enumeration) as terminating declarations.

The main disadvantage of doing things this way is making sure you won't misunderstand a ':' (or 'in')
as meaning a range-based for-loop (or fast enumeration loop) when it isn't.  The best defense IMO is to
make sure ":" is only recognized in this way at the end of a declaration (where normally a ';' would be)
(this is harder in Objective-C, but should be easier in Objective-C++).

It's worth trying to think of valid cases where you'd use a ":" inside a C for loop though.

Anyway, let me know if this makes any sense or if I missed everything.  Am I right that the beginning
of a C++ range-based for-loop is identical to a standard C for loop up until the ':' ?  If not, obviously
this technique won't work. ;-)

Thanks

Re: Range-based for in c++98

[Rodrigo Rivas]

On Mon, Oct 4, 2010 at 7:16 PM, Nicola Pero
<nicola.pero@meta-innovation.com> wrote:
> I just implemented "fast enumeration" (ie, "for (object in array) { ... }")
> for Objective-C, and I was now planning on doing it for Objective-C++ too. ;-)
>
> If you're doing range-based for-loops for C++, we may as well share ideas ;-)
Actually, they are already done. Just some minor improvements left :-)

The issue here is that I didn't feel comfortable modifying the
existing parser code of generic types just for the range-loop sake.

> Anyway, let me know if this makes any sense or if I missed everything.  Am I right that the beginning
> of a C++ range-based for-loop is identical to a standard C for loop up until the ':' ?  If not, obviously
> this technique won't work. ;-)
I don't know much about Obj-C, but I can tell that in C++ there are a
few subtle differences besides the obvious ':' vs. ';'.
C-loops may have an "expression" or a "simple-declaration" while
range-loops have a "type-specifier-seq" followed by a "declarator".
The case of an "expression" is not relevant to this discussion.

Putting it plainly, the differences are basically:
1. C-loops may declare several variables, while range-loops just one.
2. C-loops may specify a storage class (extern, static), while
range-loops may not.
3. C-loops may define new types, while range-loops may not.
4. C-loops may initialize the variables, while range-loops may not.

If these restrictions apply to the Obj-C fast-loops then the code
could be shared easily. If not, well, maybe with a little more work.

Regards.
Rodrigo

Range-based for in c++98

[Magnus Fromreide]

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

Hello.

At the moment compilation of a range-based for in c++98 fails with the
error message:

foo.cpp: In function 'int foo()':
foo.cpp:4:13: error: expected initializer before ':' token
foo.cpp:6:1: error: expected primary-expression before '}' token
foo.cpp:6:1: error: expected ';' before '}' token
foo.cpp:6:1: error: expected primary-expression before '}' token
foo.cpp:6:1: error: expected ')' before '}' token
foo.cpp:6:1: error: expected primary-expression before '}' token
foo.cpp:6:1: error: expected ';' before '}' token

This is quite unreadable and not very informative.

In order to fix this I applied a patch that gives the output:

foo.cpp: In function 'int foo()':
foo.cpp:4:11: error: range-based for only available with -std=c++0x or -std=gnu++0x

but additionally it changes the output of g++.old-deja/g++.mike/for2.C
when compiled in c++98 mode.

Here there are two problems

#1 - It doesn't change the output of g++.old-deja/g++.mike/for2.C when
     compiled in c++0x-mode
#2 - It does change the output of g++.old-deja/g++.mike/for2.C when
     compiled in c++98-mode

#1 is a problem since for2.C only contains old-style for loops and so it
is ridiculous to say things like:

for2.C:4:16: error: types may not be defined in range-based for loops

and my patch extends that behavior to the c++98 case where it makes even
less sense...

Now, the good part is that solving problem #1 should solve problem #2 as
well.

(and yes, I know I shouldn't use error but I have yet to find the
location of the colon for error_at)

/MF

[-- Attachment #2: range-based-for-diagnosis.diff --]
[-- Type: text/x-patch, Size: 910 bytes --]

Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 164381)
+++ gcc/cp/parser.c	(working copy)
@@ -8666,6 +8666,11 @@
   /* The next token should be `:'. */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_COLON))
     cp_parser_simulate_error (parser);
+  else if (cxx_dialect == cxx98)
+    {
+      error ("range-based for only available with -std=c++0x or -std=gnu++0x");
+      return NULL_TREE;
+    }
 
   /* Check if it is a range-based for */
   if (!cp_parser_parse_definitely (parser))
@@ -8922,10 +8927,7 @@
 	/* Look for the `('.  */
 	cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN);
 
-	if (cxx_dialect == cxx0x)
-	  statement = cp_parser_range_for (parser);
-	else
-	  statement = NULL_TREE;
+	statement = cp_parser_range_for (parser);
 	if (statement == NULL_TREE)
 	  statement = cp_parser_c_for (parser);
 

[-- Attachment #3: foo.cpp --]
[-- Type: text/x-c++src, Size: 61 bytes --]

int foo()
{
	int v[] = { 1 };
	for (int i : v)
		return i;
}