Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806483003:
Implement expression evaluation in Coninuation Passing Style (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 expression evaluation in Coninuation Passing Style
This enables support for Return statements.
BUG=
R=dmitryas@google.com, kmillikin@google.com
Committed: https://github.com/dart-lang/sdk/commit/feb38e4c8b15c829ffd022716653b2aaaeb36193
Patch Set 1 #Patch Set 2 : Refactor Executer placement #
Total comments: 34
Patch Set 3 : Apply comments #Patch Set 4 : Implement while, if, variable initialization in CPS #Patch Set 5 : Move recursive calls to eval in the trampoline #
Total comments: 25
Patch Set 6 : Apply comments #Messages
Total messages: 10 (2 generated)
zhivkag@google.com changed reviewers: + dmitryas@google.com, kmillikin@google.com
Hi, This change modifies the evaluation of expression from recursive to CPS and adds support for return statement and static invocations with no arguments. It introduces: - Configuration which contains the information for execution of a statement or application of expression continuation. - Different expression continuations, as we discussed. This makes the CL seem larger than it is :) We also have different representation for statement state and expression state. Any comments or suggestions are welcome. Thanks, Zhivka
I like it! However, I have some comments. Please, find them below. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:19: static StatementExecuter visitor = new StatementExecuter(); Why is it made static? https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:237: State withConfiguration(Configuration cont) { Maybe "cont" ==> "configuration"? We may use forms like "this.environment", "this.labels", and "this.expressionContinuation" as a hint that those are from the current object, but "configuration", that is shadowing the field, is not. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:264: ExpressionState.withStatementState(State state) Maybe rename it to "fromStatementState", because "with..." form is used for factory methods elsewhere. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:280: final StatementConfiguration continuation; "continuation" ==> "configuration" https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:324: final State state; I think it's better to have ExpressionState rather than State here, and use "state.statementState.configuration" (or a call to "getNextConfiguration" proposed below) as the return expression in "call" below. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:350: final Evaluator evaluator; I don't think we want to have an Evaluator in a field of an ExpressionContinuation subclass. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:371: if (state.continuation != null) { I think we may write lines 371-374 as a method "Configuration getNextConfiguration(Value value)" of ExpressionConfiguration. It will return either a new ExpressionConfiguration or the one from "statementState". https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:378: class StaticInvocationContinuation extends ExpressionContinuation { Shouldn't this class have a reference to the method being invoked and a list of arguments? https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:401: if (arguments.positional.isEmpty) { I think we may turn this if-statement into a case of ArgumentsContinuation with zero arguments, when we have ArgumentsContinuation with varying number of arguments. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:427: class VariableDeclarationContinuation extends ExpressionContinuation { I think variable declaration is not an expression, it's a statement. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:562: void trampolinedExecution(Configuration continuation) { "continuation" ==> "configuration" https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:592: StatementConfiguration cont = state.configuration; "cont" ==> "configuration" https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:605: ExpressionConfiguration cont = eval(node.condition, state); "cont" ==> "configuration" https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:605: ExpressionConfiguration cont = eval(node.condition, state); The variable "state" here is most likely a State, not ExecutionState. I think it should be transformed into an instance of ExecutionState. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:624: ExpressionConfiguration cont = eval(node.condition, state.environment); "cont" ==> "configuration" https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:624: ExpressionConfiguration cont = eval(node.condition, state.environment); Does "eval" accepts an Environment as the second argument or an ExpressionState? https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:651: var cont = new VariableDeclarationContinuation(node, state); I don't think we should have "VariableDeclarationContinuation" class. See above.
Thanks for the comments, Dima! I also modified execution of If/While/VariableDeclaration, since it was incomplete. However, we might change it further. I would prefer using continuations (like IfConditionContinuation/WhileConditionContinuation/VariableInitializerContinuation) that return the next StatementConfiguration when called in execution of While/If/VariableDeclaration statements. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:19: static StatementExecuter visitor = new StatementExecuter(); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Why is it made static? This is a remainder from some initial experiments and I forgot to remove it. Done https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:237: State withConfiguration(Configuration cont) { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Maybe "cont" ==> "configuration"? We may use forms like "this.environment", > "this.labels", and "this.expressionContinuation" as a hint that those are from > the current object, but "configuration", that is shadowing the field, is not. Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:264: ExpressionState.withStatementState(State state) On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Maybe rename it to "fromStatementState", because "with..." form is used for > factory methods elsewhere. Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:280: final StatementConfiguration continuation; On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > "continuation" ==> "configuration" Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:324: final State state; On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I think it's better to have ExpressionState rather than State here, and use > "state.statementState.configuration" (or a call to "getNextConfiguration" > proposed below) as the return expression in "call" below. Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:350: final Evaluator evaluator; On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I don't think we want to have an Evaluator in a field of an > ExpressionContinuation subclass. As we discussed offline, we couldn't see an obvious work around for not having evaluator, but we may change it later. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:371: if (state.continuation != null) { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I think we may write lines 371-374 as a method "Configuration > getNextConfiguration(Value value)" of ExpressionConfiguration. It will return > either a new ExpressionConfiguration or the one from "statementState". Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:378: class StaticInvocationContinuation extends ExpressionContinuation { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Shouldn't this class have a reference to the method being invoked and a list of > arguments? With the current implementation we only support calls without arguments, which is why StaticInvocationContinuation encapsulates only the statement or expression to be executed after the static invocation. It only needs the expression state for that. The intention was to modify this accordingly for support of invocations with arguments in another CL. In that case this class will probably have reference to arguments and/or method body. Would you prefer doing this in the current CL? https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:401: if (arguments.positional.isEmpty) { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I think we may turn this if-statement into a case of ArgumentsContinuation with > zero arguments, when we have ArgumentsContinuation with varying number of > arguments. Acknowledged. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:427: class VariableDeclarationContinuation extends ExpressionContinuation { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I think variable declaration is not an expression, it's a statement. Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:562: void trampolinedExecution(Configuration continuation) { On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > "continuation" ==> "configuration" Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:592: StatementConfiguration cont = state.configuration; On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > "cont" ==> "configuration" Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:605: ExpressionConfiguration cont = eval(node.condition, state); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > The variable "state" here is most likely a State, not ExecutionState. I think it > should be transformed into an instance of ExecutionState. Here and below are some calls to eval that I missed when changing the executer. eval has ExpressionState as second argument, I adapted accordingly, thanks! https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:605: ExpressionConfiguration cont = eval(node.condition, state); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > "cont" ==> "configuration" Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:624: ExpressionConfiguration cont = eval(node.condition, state.environment); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Does "eval" accepts an Environment as the second argument or an ExpressionState? Done. https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:624: ExpressionConfiguration cont = eval(node.condition, state.environment); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > Does "eval" accepts an Environment as the second argument or an ExpressionState? eval takes an Expression and an ExpressionState, indeed. I missed to update the execution of While and If statements according to the CPS evaluation of their condition expressions. Thanks for noticing! https://codereview.chromium.org/2806483003/diff/20001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:651: var cont = new VariableDeclarationContinuation(node, state); On 2017/04/10 14:05:53, Dmitry Stefantsov wrote: > I don't think we should have "VariableDeclarationContinuation" class. See above. Done.
I think this code is great! LTGM with some small comments. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:223: final ExpressionContinuation expressionContinuation; It think it would be easier to understand the intent of this field if it was called "enclosingExpressionContinuation". But this name is a bit too lengthy, so feel free to ignore this comment. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: final State statementState; Same as above with "expressionContinuation", I think "enclosingStatementState" is better, but a bit too lengthy. Feel free to ignore this comment.
Correcting the typo: LGTM :)
It LGTM. I have some suggestions that (I think) will make it simpler, but almost all of them can be saved for another CL. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:78: class Evaluator extends ExpressionVisitor1<Configuration> { We should make ExpressionVisitor1 parameterized over the type of the extra argument as well as the return type. That can help with type-based refactorings, for instance. Feel free to make that a separate change. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:79: Configuration eval(Expression expr, ExpressionState state) => ExpressionState is ExpressionConfiguration without the expression so there is an 'isomorphism' between (Expression, ExpressionState) and ExpressionConfiguration. I think it probably reduces boilerplate to pass the ExpressionConfiguration itself, including the redundant expression, to eval and all the Visit methods. That can be a separate change. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:93: return state.nextConfiguration(value); I think nextConfiguration is only needed because the continuation can be null (representing the continuation of the expression in an expression statement). Instead, I suggest making it explicit by having: // There's probably a better name. class ExpressionStatementContinuation extends ExpressionContinuation { final StatementConfiguration configuration; ExpressionStatementContinuation(this.configuration); Configuration call(Value _) { return configuration; } } Then we can eliminate nextConfiguration and write explicitly: return new ContinuationConfiguration(value, state.continuation); here and similarly elsewhere. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:128: state.statementState This doesn't seem like the right StatementState here. It should: - Have an empty or top-level environment, because bindings in the caller are not accessible to the callee. - Have an empty list of break labels, because we can't break from the callee to a label in the caller. - Have an "unreachable" statement continuation (the configuration component of the state) because using the statement continuation of the caller would represent the ability to abort an expression context to the nearest enclosing statement, from within a called function. Something like: class UnreachableConfiguration extends Configuration implements StatementConfiguration { Configuration step(StatementExecuter executer) { throw 'UNREACHABLE'; } } If we change this and make the earlier suggested change of explicitly representing the context of the expression in an ExpressionStatement (instead of using null and picking the statement configuration out of the ExpressionState), then I don't think we need to have the (Statement)State in the ExpressionState at all. (Which is comforting, because expressions should not have access to any of it except the exception handlers when we add them.) https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:221: final Configuration configuration; This is always a StatementConfiguration isn't it (provided that the UnreachableConfiguration I suggested above is a StatementConfiguration)? Maybe we should type it that way. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:223: final ExpressionContinuation expressionContinuation; On 2017/04/12 12:53:11, Dmitry Stefantsov wrote: > It think it would be easier to understand the intent of this field if it was > called "enclosingExpressionContinuation". But this name is a bit too lengthy, so > feel free to ignore this comment. It's 'returnContinuaton', which seems like a good name. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: final State statementState; On 2017/04/12 12:53:11, Dmitry Stefantsov wrote: > Same as above with "expressionContinuation", I think "enclosingStatementState" > is better, but a bit too lengthy. Feel free to ignore this comment. I think it's unnecessary as described above. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:324: ContinuationConfiguration(this.value, this.continuation); I prefer the argument order to be swapped here (swap the field declarations, too) because then all the configurations have the nice property that their first component is the one that we dispatch on when defining the next configuration. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:342: ExpressionState get state; I'm not sure that every expression continuation should have an ExpressionState. That includes at least an environment and a continuation, but we don't always need both or either of them. I suggest removing it from here and including just the components that are needed for the individual continuations. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:365: Value propertyValue = receiver.class_.lookupGetter(name)(receiver); Not for this change, but we will have to find a way to CPS the getters (and setters) because they can include user code which means they have to be able to return (and throw). https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:378: Setter setter = receiver.class_.lookupSetter(setterName); The spec has that setters are looked up after evaluating the right-hand side. I don't know that it makes much difference here but it might be clearer to move it into the application of the SetterContinuation. That's nice because then the SetterContinuation is first-order (it has a name instead of a function).
Thanks for the review! I applied most of the suggestions, the remaining will be separate CLs. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... File pkg/kernel/lib/interpreter/interpreter.dart (right): https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:78: class Evaluator extends ExpressionVisitor1<Configuration> { On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > We should make ExpressionVisitor1 parameterized over the type of the extra > argument as well as the return type. > > That can help with type-based refactorings, for instance. Feel free to make > that a separate change. Acknowledged. I will do it in a follow-up CL. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:79: Configuration eval(Expression expr, ExpressionState state) => On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > ExpressionState is ExpressionConfiguration without the expression so there is an > 'isomorphism' between (Expression, ExpressionState) and ExpressionConfiguration. > > I think it probably reduces boilerplate to pass the ExpressionConfiguration > itself, including the redundant expression, to eval and all the Visit methods. > > That can be a separate change. Acknowledged. I will do it in a follow-up CL. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:93: return state.nextConfiguration(value); On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > I think nextConfiguration is only needed because the continuation can be null > (representing the continuation of the expression in an expression statement). > Instead, I suggest making it explicit by having: > > // There's probably a better name. > class ExpressionStatementContinuation extends ExpressionContinuation { > final StatementConfiguration configuration; > > ExpressionStatementContinuation(this.configuration); > > Configuration call(Value _) { > return configuration; > } > } > > Then we can eliminate nextConfiguration and write explicitly: > > return new ContinuationConfiguration(value, state.continuation); > > here and similarly elsewhere. Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:128: state.statementState On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > This doesn't seem like the right StatementState here. It should: > > - Have an empty or top-level environment, because bindings in the caller are not > accessible to the callee. > > - Have an empty list of break labels, because we can't break from the callee to > a label in the caller. > > - Have an "unreachable" statement continuation (the configuration component of > the state) because using the statement continuation of the caller would > represent the ability to abort an expression context to the nearest enclosing > statement, from within a called function. > > Something like: > > class UnreachableConfiguration extends Configuration > implements StatementConfiguration { > Configuration step(StatementExecuter executer) { > throw 'UNREACHABLE'; > } > } > > > If we change this and make the earlier suggested change of explicitly > representing the context of the expression in an ExpressionStatement (instead of > using null and picking the statement configuration out of the ExpressionState), > then I don't think we need to have the (Statement)State in the ExpressionState > at all. (Which is comforting, because expressions should not have access to any > of it except the exception handlers when we add them.) Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:221: final Configuration configuration; On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > This is always a StatementConfiguration isn't it (provided that the > UnreachableConfiguration I suggested above is a StatementConfiguration)? > > Maybe we should type it that way. Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:223: final ExpressionContinuation expressionContinuation; On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > On 2017/04/12 12:53:11, Dmitry Stefantsov wrote: > > It think it would be easier to understand the intent of this field if it was > > called "enclosingExpressionContinuation". But this name is a bit too lengthy, > so > > feel free to ignore this comment. > > It's 'returnContinuaton', which seems like a good name. Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: final State statementState; On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > On 2017/04/12 12:53:11, Dmitry Stefantsov wrote: > > Same as above with "expressionContinuation", I think "enclosingStatementState" > > is better, but a bit too lengthy. Feel free to ignore this comment. > > I think it's unnecessary as described above. Acknowledged. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:257: final State statementState; On 2017/04/12 12:53:11, Dmitry Stefantsov wrote: > Same as above with "expressionContinuation", I think "enclosingStatementState" > is better, but a bit too lengthy. Feel free to ignore this comment. Acknowledged. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:324: ContinuationConfiguration(this.value, this.continuation); On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > I prefer the argument order to be swapped here (swap the field declarations, > too) because then all the configurations have the nice property that their first > component is the one that we dispatch on when defining the next configuration. Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:342: ExpressionState get state; On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > I'm not sure that every expression continuation should have an ExpressionState. > That includes at least an environment and a continuation, but we don't always > need both or either of them. > > I suggest removing it from here and including just the components that are > needed for the individual continuations. Done. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:365: Value propertyValue = receiver.class_.lookupGetter(name)(receiver); On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > Not for this change, but we will have to find a way to CPS the getters (and > setters) because they can include user code which means they have to be able to > return (and throw). Indeed. This is sufficient for now only because we have implicit getters/setters, but this will change. Adding a TODO. https://codereview.chromium.org/2806483003/diff/80001/pkg/kernel/lib/interpre... pkg/kernel/lib/interpreter/interpreter.dart:378: Setter setter = receiver.class_.lookupSetter(setterName); On 2017/04/20 10:50:20, Kevin Millikin (Google) wrote: > The spec has that setters are looked up after evaluating the right-hand side. I > don't know that it makes much difference here but it might be clearer to move it > into the application of the SetterContinuation. > > That's nice because then the SetterContinuation is first-order (it has a name > instead of a function). Done.
Description was changed from ========== Implement expression evaluation in Coninuation Passing Style This enables support for Return statements. BUG= ========== to ========== Implement expression evaluation in Coninuation Passing Style This enables support for Return statements. BUG= R=dmitryas@google.com, kmillikin@google.com Committed: https://github.com/dart-lang/sdk/commit/feb38e4c8b15c829ffd022716653b2aaaeb36193 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as feb38e4c8b15c829ffd022716653b2aaaeb36193 (presubmit successful). |
