First stab at parsing redirecting constructors.

First stab at parsing redirecting constructors.

Committed: https://code.google.com/p/dart/source/detail?r=13410

[Lasse Reichstein Nielsen, 2012-09-24 13:06:41]

Is it something like this we need?

[Lasse Reichstein Nielsen, 2012-09-24 13:48:37]

Nope, not good enough - doesn't handle
  factory C() => foo.Bar<T,int>.baz;
It must parse not just a type, but anything that can go after a "new" (and
before the following '(').

[Lasse Reichstein Nielsen, 2012-09-24 13:49:15]

That should ofcourse be:
 factory C() = foo.Bar<T,int>.baz;

[Lasse Reichstein Nielsen, 2012-09-26 07:45:12]

Please take a look.
This should parse the correct syntax (and then bail-out in the resolver, because
we don't handle it yet).

[ahe, 2012-09-26 10:42:55]

http://codereview.chromium.org/10957060/diff/5001/lib/compiler/implementation...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10957060/diff/5001/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:998: Token
parseRedirectingFactoryBody(Token token) {
listener.beginFoo

http://codereview.chromium.org/10957060/diff/5001/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:1013:
listener.endReturnStatement(true, equals, token);
Just one listener.endFoo.

[Lasse Reichstein Nielsen, 2012-09-26 14:36:05]

PTAL

[ahe, 2012-09-26 14:51:09]

Much better. But I got more ideas :-)

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1630: if (node.beginToken.stringValue
=== '=') {
I think we should have a method/getter for this.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1631: NewExpression newExpression =
node.expression;
I think we should just say that this is an error, and assume this situation was
already handled around line 150 and resolveConstructorImplementation.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/listener.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:256: void
endRedirectingFactoryBody(Token start, Token dot, Token end) {
Not sure this is "dot". You're using a different name below.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:1166: void
endRedirectingFactoryBody(Token start, Token namePartStart, Token end) {
What is namePartStart? Should it be periodBeforeName (as used elsewhere in this
file)?

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:1167: // This generates code
for '= Type<T>.foo' that is similar to
code -> nodes

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1008: expectSemicolon(token);
You're not consuming the semicolon token. Is that intentional?

[Lasse Reichstein Nielsen, 2012-09-27 11:50:59]

PTAL

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1630: if (node.beginToken.stringValue
=== '=') {
On 2012/09/26 14:51:09, ahe wrote:
> I think we should have a method/getter for this.

Done.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1631: NewExpression newExpression =
node.expression;
Done. No just calls unimplemented immediately.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/listener.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:256: void
endRedirectingFactoryBody(Token start, Token dot, Token end) {
Switched to periodBeforeName everywhere.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:1166: void
endRedirectingFactoryBody(Token start, Token namePartStart, Token end) {
On 2012/09/26 14:51:09, ahe wrote:
> What is namePartStart? Should it be periodBeforeName (as used elsewhere in
this
> file)?

Done.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/listener.dart:1167: // This generates code
for '= Type<T>.foo' that is similar to
On 2012/09/26 14:51:09, ahe wrote:
> code -> nodes

Done.

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10957060/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1008: expectSemicolon(token);
Yes, I act the same as the '=>' part of parseFunctionBody. The semicolon will be
skipped by the "return token.next;" in parseFactoryMethod.

[Lasse Reichstein Nielsen, 2012-10-05 08:10:30]

Ping too.

[ahe, 2012-10-05 08:23:19]

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/listener.dart:1183: new NewExpression(start,
new Send(null, target, noArguments));
We can't have the same token in two AST node.

I think you need to test this in dart/tests/compiler/dart2js/unparser_test.dart

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1000: assert('=' ===
token.stringValue);
assert(optional('=', token));

I should clean up the other uses similar to this.

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1010: return token;
You should consume the semicolon token if you require it.

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:894: String get
firstTokenStringValue => beginToken.stringValue;
Where is this used?

[Lasse Reichstein Nielsen, 2012-10-09 09:46:54]

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/listener.dart:1183: new NewExpression(start,
new Send(null, target, noArguments));
On 2012/10/05 08:23:20, ahe wrote:
> We can't have the same token in two AST node.

That's ... problematic. We might need a new AST node then.

> I think you need to test this in
dart/tests/compiler/dart2js/unparser_test.dart

Will do.

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1000: assert('=' ===
token.stringValue);
On 2012/10/05 08:23:20, ahe wrote:
> assert(optional('=', token));
> 
> I should clean up the other uses similar to this.

Done.

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1010: return token;
I don't think that's compatible with parseFunctionBody.
In parseFunctionBody, the semicolon is not consumed, and the calling methods use
the returned token (the semicolon) as the end-token of their calls to
endFunction. The parseRedirectingFactoryBody method is used in the same place as
a call to parseFunctionBody, and seems to need the same behavior.

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10957060/diff/11005/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:894: String get
firstTokenStringValue => beginToken.stringValue;
In unparser, validator and resolver. Looking at the beginToken seems to happen a
lot.

Would it be better to have methods on Return like "isFunctionBody" and
"isRedirectingFactoryBody" that does the check?

[ahe, 2012-10-09 10:51:37]

LGTM

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/listener.dart:1661: for (;;) {
Could you unfold this loop so we can easily see that it terminates.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1054: Token
parseQualifiedPart(Token token) {
Why is this a nested function?

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1075:
listener.beginRedirectingFactoryBody(token);
Move this before the assert.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:925: String get
firstTokenStringValue => beginToken.stringValue;
You don't need this, you should have an accessor that answers
"isRedirectingConstructorBody"

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/tree/unparser.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/tree/unparser.dart:240: sb.add(' ');
Why?

[Lasse Reichstein Nielsen, 2012-10-09 11:51:26]

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/listener.dart:1661: for (;;) {
On 2012/10/09 10:51:37, ahe wrote:
> Could you unfold this loop so we can easily see that it terminates.

Done.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1054: Token
parseQualifiedPart(Token token) {
because I didn't want to duplicate the code, but it has no obvious use outside
this function.
If I had a better loop construct, I wouldn't need it.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1075:
listener.beginRedirectingFactoryBody(token);
On 2012/10/09 10:51:37, ahe wrote:
> Move this before the assert.

Done.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/tree/nodes.dart:925: String get
firstTokenStringValue => beginToken.stringValue;
On 2012/10/09 10:51:37, ahe wrote:
> You don't need this, you should have an accessor that answers
> "isRedirectingConstructorBody"

Done.

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/tree/unparser.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/tree/unparser.dart:240: sb.add(' ');
Looks prettier.
We bother putting a space after the '=>', so we should also ensure that there
are spaces around the '='.

[ahe, 2012-10-09 12:16:51]

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
File lib/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/10957060/diff/23001/lib/compiler/implementati...
lib/compiler/implementation/scanner/parser.dart:1054: Token
parseQualifiedPart(Token token) {
On 2012/10/09 11:51:26, Lasse Reichstein Nielsen wrote:
> because I didn't want to duplicate the code, but it has no obvious use outside
> this function.

There is no reason to create extra overhead by creating a closure each time.
Please don't do it in the parser.

Also, there is always a use for the function outside this method. It is not like
we have tried keeping the number of parseFoo methods small.

> If I had a better loop construct, I wouldn't need it.