Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790063002:
Implement flow control with breaks (Closed)
|
Created:
3 years, 8 months ago by zhivkag Modified:
3 years, 8 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplement flow control with breaks
BUG=
R=dmitryas@google.com, kmillikin@google.com
Committed: https://github.com/dart-lang/sdk/commit/c91fd3103641fc7629f6902dccdf7cf95d9147a8
Patch Set 1 #Patch Set 2 : Fix comment #Patch Set 3 : Refactor #Patch Set 4 : Fix bad refactoring #Patch Set 5 : Implement breaks with linked states, conts and labels #
Total comments: 8
Patch Set 6 : Apply comments #
Total comments: 20
Patch Set 7 : Apply comments #Messages
Total messages: 9 (2 generated)
zhivkag@google.com changed reviewers: + dmitryas@google.com, kmillikin@google.com
Hi, This is an implementation for breaks, as we discussed. It introduces: - State, which contains an Environment, Label and a Continuation - Continuation, which consists of the statement to be executed and the state for the execution. - Label, which is a linked list containing a LabeledStatement (the target of a BreakStatement) and a Continuation. The labeled statement is used as a key to determine the corresponding Continuation to return when a break statement is executed. The execution of statements in a program proceeds as follows: an execution of a statement returns a Continuation. The execution completes when the empty Continuation is returned, otherwise the returned Continuation is applied. Any comments or suggestions are welcome. Thanks, Zhivka
Great code! LGTM with few comments. https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:226: State.fromEnvironment(Environment env, State s) It's probably nitpicking, but 'fromEnvironment' looks more like 'withEnvironment' for me, as in the phrase "copy this state with this environment". Same for 'fromContinuation' below. Please feel free to ignore this comment :) https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: /// enclosing labeled statement. Maybe "labeled statement" ==> "label"? https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:261: final Label label; Maybe "label" ==> "enclosingLabel"? https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:293: Continuation exec(Statement statement, cont) => statement.accept1(this, cont); From the code below it looks like "cont" should be replaced with "state" here.
https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:226: State.fromEnvironment(Environment env, State s) On 2017/04/03 14:17:20, Dmitry Stefantsov wrote: > It's probably nitpicking, but 'fromEnvironment' looks more like > 'withEnvironment' for me, as in the phrase "copy this state with this > environment". Same for 'fromContinuation' below. Please feel free to ignore this > comment :) Done. https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: /// enclosing labeled statement. On 2017/04/03 14:17:20, Dmitry Stefantsov wrote: > Maybe "labeled statement" ==> "label"? Done. https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:261: final Label label; On 2017/04/03 14:17:20, Dmitry Stefantsov wrote: > Maybe "label" ==> "enclosingLabel"? Done. https://codereview.chromium.org/2790063002/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:293: Continuation exec(Statement statement, cont) => statement.accept1(this, cont); On 2017/04/03 14:17:20, Dmitry Stefantsov wrote: > From the code below it looks like "cont" should be replaced with "state" here. Indeed, I have missed to rename this (and some others) from the previous patches - thanks!
This looks very good. I have a lot of comments but they are somewhat superficial and can be easily adopted later. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:223: Label label; List these in the same order as the constructor arguments for uniformity. I think we should call env 'environmment' since we've spelled out continuation or else we should call continuation 'cont'. I prefer spelling fields out in full, but I'm happy to abbreviate parameter names. 'label' could be 'labels' because I think of it as plural: it's a stack or linked list of (statement, continuation) pairs. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:226: State.withEnvironment(Environment env, State s) This is basically a static function that we have to invoke with new State.withEnvironment. Since it has a State argument already, I think it might be nicer to have it be a method: State withEnvironment(Environment env) { return new State(env, label, cont); } https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:232: State.empty() Empty state seems ambiguous. Perhaps State.initial: State.initial() : this(new Environment.empty(), null, new Continuation.initial()); https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:237: label = new Label(s, continuation, label); Suggest to make label be final and use a constructor (or method as above): State withBreak(Statement stmt) { return new State(env, new Label(stmt, cont, labels), cont); } https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:251: const Continuation.empty() This is the initial continuation, right? I think we have the property that bodies always have a return, in which case we do not expect to 'invoke' this continuation (by ending up trying to evaluate the statement). Perhaps it is Continuation.error, in that case? For now I think we could just use null. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:264: Label.empty() Does it make sense to use null as the sentinel for the end of the list? new Label.empty().lookup(s) doesn't work for any s except null, which we shouldn't ask. It dodges the issue that we have the same continuation as the initial continuation, which doesn't seem quite right. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:271: return enclosingLabel.lookupLabel(s); In a well-formed program we will not get that enclosingLabel is null, but maybe we should assert it. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:315: Continuation cont; Maybe it's a little hard to reason about this code because blockState is mutated on each iteration. All the states are the same except that they differ in the continuation, so I think it's clearer to just change the continuation: Continuation cont = state.continuation; for (Statement stmt in node.statements.reversed) { cont = new Continuation(stmt, blockState.withContinuation(cont)); } return cont; We might also consider short-circuiting the transition exec({s0, ...}, state) -> Continuation(s0, state') -> exec(s0, state') which would look something like: if (node.statements.isEmpty) return state.continuation; State blockState = new State.withEnvironment(new Environment(state.environment)); // Construct the continuation of the first statement. Continuation cont = state.continuation; for (Statement stmt in node.statements.skip(1).reversed) { cont = new Continuation(stmt, blockState.withContinuation(cont)); } return exec(node.statements.first, blockState.withContinuation(cont)); https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:330: return new Continuation(node.then, state); Here we can (should?) have: return exec(node.then, state); https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:352: return new Continuation(node.body, new State.withContinuation(c, state)); return exec(node.body, state.withContinuation(c));
Thanks for the comments! As discussed, I kept the execution of statements in the trampoline even though in some cases we can shortcut and directly call exec instead of returning new Continuation(..). https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:223: Label label; On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > List these in the same order as the constructor arguments for uniformity. I > think we should call env 'environmment' since we've spelled out continuation or > else we should call continuation 'cont'. > > I prefer spelling fields out in full, but I'm happy to abbreviate parameter > names. > > 'label' could be 'labels' because I think of it as plural: it's a stack or > linked list of (statement, continuation) pairs. Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:226: State.withEnvironment(Environment env, State s) On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > This is basically a static function that we have to invoke with new > State.withEnvironment. Since it has a State argument already, I think it might > be nicer to have it be a method: > > State withEnvironment(Environment env) { > return new State(env, label, cont); > } Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:232: State.empty() On 2017/04/05 08:21:48, Kevin Millikin (Google) wrote: > Empty state seems ambiguous. Perhaps State.initial: > > State.initial() : this(new Environment.empty(), null, new > Continuation.initial()); Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:237: label = new Label(s, continuation, label); On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > Suggest to make label be final and use a constructor (or method as above): > > State withBreak(Statement stmt) { > return new State(env, new Label(stmt, cont, labels), cont); > } Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:251: const Continuation.empty() On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > This is the initial continuation, right? I think we have the property that > bodies always have a return, in which case we do not expect to 'invoke' this > continuation (by ending up trying to evaluate the statement). Perhaps it is > Continuation.error, in that case? > > For now I think we could just use null. Yes, we shouldn't apply this continuation. exec should return it only when the entire program was executed. Using null for now and we can change it later to Continuation.error or something else if needed. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:264: Label.empty() On 2017/04/05 08:21:48, Kevin Millikin (Google) wrote: > Does it make sense to use null as the sentinel for the end of the list? new > Label.empty().lookup(s) doesn't work for any s except null, which we shouldn't > ask. > > It dodges the issue that we have the same continuation as the initial > continuation, which doesn't seem quite right. Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:271: return enclosingLabel.lookupLabel(s); On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > In a well-formed program we will not get that enclosingLabel is null, but maybe > we should assert it. Done. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:315: Continuation cont; On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > Maybe it's a little hard to reason about this code because blockState is mutated > on each iteration. All the states are the same except that they differ in the > continuation, so I think it's clearer to just change the continuation: > > Continuation cont = state.continuation; > for (Statement stmt in node.statements.reversed) { > cont = new Continuation(stmt, blockState.withContinuation(cont)); > } > return cont; Done. > > > We might also consider short-circuiting the transition > > exec({s0, ...}, state) -> Continuation(s0, state') -> exec(s0, state') > > which would look something like: > > if (node.statements.isEmpty) return state.continuation; > > State blockState = > new State.withEnvironment(new Environment(state.environment)); > // Construct the continuation of the first statement. > Continuation cont = state.continuation; > for (Statement stmt in node.statements.skip(1).reversed) { > cont = new Continuation(stmt, blockState.withContinuation(cont)); > } > return exec(node.statements.first, blockState.withContinuation(cont)); Acknowledged. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:330: return new Continuation(node.then, state); On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > Here we can (should?) have: > > return exec(node.then, state); Acknowledged. https://codereview.chromium.org/2790063002/diff/100001/pkg/kernel/lib/interpr... pkg/kernel/lib/interpreter/interpreter.dart:352: return new Continuation(node.body, new State.withContinuation(c, state)); On 2017/04/05 08:21:47, Kevin Millikin (Google) wrote: > return exec(node.body, state.withContinuation(c)); Acknowledged.
LGTM.
Description was changed from ========== Implement flow control with breaks BUG= ========== to ========== Implement flow control with breaks BUG= R=dmitryas@google.com, kmillikin@google.com Committed: https://github.com/dart-lang/sdk/commit/c91fd3103641fc7629f6902dccdf7cf95d9147a8 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as c91fd3103641fc7629f6902dccdf7cf95d9147a8 (presubmit successful). |
