Implement cascaded calls.

Implement cascaded calls.

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

[sra1, 2012-03-30 21:52:19]

https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/...
File frog/tests/leg_only/src/CascadeTest.dart (right):

https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/...
frog/tests/leg_only/src/CascadeTest.dart:43: ..check(2,
10)..setX(10).setY(3)..check(10, 3)
Could you reformat it so that the assignment forms are not all on the right?
Otherwise you are not testing that .. adjacent to a number compiles.
E.g.

  a..check(1, 2)..swap()..check(2, 1)
   ..x = 4..check(4, 1)..setX(10)..check(10, 1)
   ..y = 5..check(10, 5)..swap()()()
   ..check(5, 10)..[0] = 2..check(2, 10)
   ..setX(10).setY(3)..check(10, 3)
   ..["swap"]()()()..check(3, 10);

[Lasse Reichstein Nielsen, 2012-04-10 08:12:43]

https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/...
File frog/tests/leg_only/src/CascadeTest.dart (right):

https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/...
frog/tests/leg_only/src/CascadeTest.dart:43: ..check(2,
10)..setX(10).setY(3)..check(10, 3)
Good point. 
I wasn't expecting it to work, but that's just JS-related mental damage.
(In JS, "2." is a valid number literal, so you have to write "2..toString()" to
convert it to a string).

[Lasse Reichstein Nielsen, 2012-04-10 12:31:27]

https://chromiumcodereview.appspot.com/9958009/diff/7001/lib/compiler/impleme...
File lib/compiler/implementation/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/9958009/diff/7001/lib/compiler/impleme...
lib/compiler/implementation/ssa/closure.dart:197: 
Oops, bad edit.

[Lasse Reichstein Nielsen, 2012-04-11 06:56:08]

Addijng floitsch as reviewer while ngeoffray is absent.

[ahe, 2012-04-16 08:55:23]

LGTM if you address my comments (now or in a follow up).

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
File frog/tests/leg_only/src/CascadeTest.dart (right):

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:11: A setX(int x) { this.x = x; return
this; }
Would be nice with a single empty line between the various functions, accessors,
and constructors.

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:49: ..["swap"]()()()..check(7, 3);
I think this test will pass if cascades are no-ops, so how about adding:

a.check(7, 3);

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:50: a..(42);  /// 01: compile-time
error
Additional suggestions for tests:

a..42; /// 02: compile-time error
a..""; /// 03: compile-time error

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/listener.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/listener.dart:1019: if (token.stringValue
=== '.' || token.stringValue == '..') {
Please store the stringValue in a temporary.

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

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:924: Token next = expect('..',
token);
I normally use:

  assert(optional('..', token))

when the token is already checked before calling.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:925:
listener.beginCascade(token);
In general, it is best to call the begin method before calling expect. This way
the listener knows what is going on and has a better chance at recovering from
the error. However, in this case, it is not necessary to call expect.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:927: next =
parseArgumentOrIndexStar(next);
Normally, I use this pattern:

token = parse...

This is deliberate, as I assume token gets assigned a register. If the incoming
token is needed later, I give it a name. For example, see
parseConditionalExpressionRest above.

Ignoring the register guesswork, I'd like the code to be consistent. This means
that the variable named "token" always holds the next token.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:928: } else if (next.kind ==
IDENTIFIER_TOKEN) {
What if it is a pseudo keyword? Perhaps you should use "isIdentifier(next)".

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:930:
listener.handleBinaryExpression(token);
For example, this sequence is confusing (to me) because token doesn't mean what
it does in other parts of the parser.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:938: Token dot = next;
I have used the word "period" instead of "dot" in other places.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:943: } while (mark != next);
!==

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:945: if (next.info.precedence ==
ASSIGNMENT_PRECEDENCE) {
===

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:947: next =
parsePrecedenceExpression(next.next, SEQUENCE_PRECEDENCE + 1);
Question to make sure I understand what is going on here: I guess this means
that you cannot write:

x..y = a = b;

Perhaps you should test that?

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/scanner.dart:17: abstract void
appendPrecedenceToken(PrecedenceInfo info);
Thank you for fixing this. It is easier to review code if you do such
maintenance tasks as a separate CL.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/scanner.dart:187: return
tokenizeDotsOrNumber(next);
This is actually not consistent with the naming scheme I have use elsewhere. For
example, tokenizeBar and tokenizeAmpersand, not tokenizeBars and
tokenizeAmpersands.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/token.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/token.dart:251: // Sequence operator has the
lowest precedence.
There is no sequence operator in the Dart language. I'd like a more descriptive
name.

Also, please use documentation comments.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/ssa/builder.dart:2282: stack.add(stack.last());
Perhaps add a comment or create a dup() method.

[Lasse Reichstein Nielsen, 2012-04-16 12:41:38]

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
File frog/tests/leg_only/src/CascadeTest.dart (right):

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:11: A setX(int x) { this.x = x; return
this; }
On 2012/04/16 08:55:23, ahe wrote:
> Would be nice with a single empty line between the various functions,
accessors,
> and constructors.

Done.

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:49: ..["swap"]()()()..check(7, 3);
Good point. Done.

http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/Casc...
frog/tests/leg_only/src/CascadeTest.dart:50: a..(42);  /// 01: compile-time
error
On 2012/04/16 08:55:23, ahe wrote:
> Additional suggestions for tests:
> 
> a..42; /// 02: compile-time error
> a..""; /// 03: compile-time error

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/listener.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/listener.dart:1019: if (token.stringValue
=== '.' || token.stringValue == '..') {
On 2012/04/16 08:55:23, ahe wrote:
> Please store the stringValue in a temporary.

Done.

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

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:924: Token next = expect('..',
token);
On 2012/04/16 08:55:23, ahe wrote:
> I normally use:
> 
>   assert(optional('..', token))
> 
> when the token is already checked before calling.

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:925:
listener.beginCascade(token);
On 2012/04/16 08:55:23, ahe wrote:
> In general, it is best to call the begin method before calling expect. This
way
> the listener knows what is going on and has a better chance at recovering from
> the error. However, in this case, it is not necessary to call expect.

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:927: next =
parseArgumentOrIndexStar(next);
On 2012/04/16 08:55:23, ahe wrote:
> Normally, I use this pattern:
> 
> token = parse...
> 
> This is deliberate, as I assume token gets assigned a register. If the
incoming
> token is needed later, I give it a name. For example, see
> parseConditionalExpressionRest above.
> 
> Ignoring the register guesswork, I'd like the code to be consistent. This
means
> that the variable named "token" always holds the next token.

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:928: } else if (next.kind ==
IDENTIFIER_TOKEN) {
On 2012/04/16 08:55:23, ahe wrote:
> What if it is a pseudo keyword? Perhaps you should use "isIdentifier(next)".

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:930:
listener.handleBinaryExpression(token);
Changed to have a more readable name for token.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:938: Token dot = next;
On 2012/04/16 08:55:23, ahe wrote:
> I have used the word "period" instead of "dot" in other places.

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:943: } while (mark != next);
On 2012/04/16 08:55:23, ahe wrote:
> !==

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:945: if (next.info.precedence ==
ASSIGNMENT_PRECEDENCE) {
On 2012/04/16 08:55:23, ahe wrote:
> ===

Done.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/parser.dart:947: next =
parsePrecedenceExpression(next.next, SEQUENCE_PRECEDENCE + 1);
You should be able to write that. It would parse as
 x..y = (a = b)
(which is fine, since assignment is right-associative).

This means that you cannot have a cascade om the expression after the
assignment-operator.
SEQUENCE is just the wrong name, I'll change it to CASCADE.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/scanner.dart:17: abstract void
appendPrecedenceToken(PrecedenceInfo info);
True.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/scanner/token.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/scanner/token.dart:251: // Sequence operator has the
lowest precedence.
Changed "sequence" to "cascade" - which is what it should always have been. I
just confused myself in the beginning.

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/9958009/diff/7001/lib/compiler/implementation/...
lib/compiler/implementation/ssa/builder.dart:2282: stack.add(stack.last());
Added dup().