Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 9958009: Implement cascaded calls. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 1 month ago by Lasse Reichstein Nielsen
Modified:
5 years, 1 month ago
Reviewers:
ahe, floitsch, ngeoffray, sra1
CC:
reviews_dartlang.org, ahe
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Proof-of-concept implementation of cascade #

Total comments: 2

Patch Set 3 : Update tests. Fix scanner. #

Total comments: 34

Patch Set 4 : Address review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+254 lines, -45 lines) Patch
A frog/tests/leg_only/src/CascadeTest.dart View 1 2 3 1 chunk +69 lines, -0 lines 0 comments Download
M lib/compiler/implementation/resolver.dart View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M lib/compiler/implementation/scanner/array_based_scanner.dart View 1 1 chunk +3 lines, -3 lines 0 comments Download
M lib/compiler/implementation/scanner/listener.dart View 1 2 3 3 chunks +16 lines, -1 line 0 comments Download
M lib/compiler/implementation/scanner/parser.dart View 1 2 3 5 chunks +37 lines, -4 lines 0 comments Download
M lib/compiler/implementation/scanner/scanner.dart View 1 2 19 chunks +37 lines, -34 lines 0 comments Download
M lib/compiler/implementation/scanner/token.dart View 1 2 3 2 chunks +7 lines, -3 lines 0 comments Download
M lib/compiler/implementation/ssa/builder.dart View 1 2 3 2 chunks +15 lines, -0 lines 0 comments Download
M lib/compiler/implementation/tree/nodes.dart View 1 2 3 3 chunks +37 lines, -0 lines 0 comments Download
M lib/compiler/implementation/tree/visitors.dart View 1 1 chunk +2 lines, -0 lines 0 comments Download
M lib/compiler/implementation/typechecker.dart View 1 2 2 chunks +23 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 8 (0 generated)
Lasse Reichstein Nielsen
5 years, 1 month ago (2012-03-30 11:35:27 UTC) #1
Lasse Reichstein Nielsen
5 years, 1 month ago (2012-03-30 11:35:50 UTC) #2
sra1
https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/src/CascadeTest.dart File frog/tests/leg_only/src/CascadeTest.dart (right): https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/src/CascadeTest.dart#newcode43 frog/tests/leg_only/src/CascadeTest.dart:43: ..check(2, 10)..setX(10).setY(3)..check(10, 3) Could you reformat it so that ...
5 years, 1 month ago (2012-03-30 21:52:19 UTC) #3
Lasse Reichstein Nielsen
https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/src/CascadeTest.dart File frog/tests/leg_only/src/CascadeTest.dart (right): https://chromiumcodereview.appspot.com/9958009/diff/2001/frog/tests/leg_only/src/CascadeTest.dart#newcode43 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 ...
5 years, 1 month ago (2012-04-10 08:12:43 UTC) #4
Lasse Reichstein Nielsen
https://chromiumcodereview.appspot.com/9958009/diff/7001/lib/compiler/implementation/ssa/closure.dart File lib/compiler/implementation/ssa/closure.dart (right): https://chromiumcodereview.appspot.com/9958009/diff/7001/lib/compiler/implementation/ssa/closure.dart#newcode197 lib/compiler/implementation/ssa/closure.dart:197: Oops, bad edit.
5 years, 1 month ago (2012-04-10 12:31:27 UTC) #5
Lasse Reichstein Nielsen
Addijng floitsch as reviewer while ngeoffray is absent.
5 years, 1 month ago (2012-04-11 06:56:08 UTC) #6
ahe
LGTM if you address my comments (now or in a follow up). http://codereview.chromium.org/9958009/diff/7001/frog/tests/leg_only/src/CascadeTest.dart File frog/tests/leg_only/src/CascadeTest.dart ...
5 years, 1 month ago (2012-04-16 08:55:23 UTC) #7
Lasse Reichstein Nielsen
5 years, 1 month ago (2012-04-16 12:41:38 UTC) #8
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().
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06