Use an inner object instead of captured vars. (issue 7547045)

[a...@chromium.org]

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/GeneratorTransformer.js
File src/codegeneration/generator/GeneratorTransformer.js (right):

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/GeneratorTransformer.js#newcode221
src/codegeneration/generator/GeneratorTransformer.js:221: ${GSTATE}:
${ST_NEWBORN},
Now we don't really need placeholders for the key names. Just do:

var ${G} = {
state: ...,
current: ....,
yieldReturn: ...,
moveNext: ...
};

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/YieldState.js
File src/codegeneration/generator/YieldState.js (right):

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/YieldState.js#newcode64
src/codegeneration/generator/YieldState.js:64: // this.$current =
expression;
What is this here? I guess I should apply the patch and see what is
generated.

https://codereview.appspot.com/7547045/

[usrb...@yahoo.com]

Reviewers: arv-chromium,

Message:
Note: As a matter of principle and convenience (reviewers and me alike),
I'm just making sure there's one copy in every patch in the series.

Patch order at a glance:

Make generator common code depend only upon constants.
https://codereview.appspot.com/7761043
Use generator 'return' to simplify the code for 'close()'
https://codereview.appspot.com/7519046
Checking undefined properties is slow, so cache in a boolean.
https://codereview.appspot.com/7663044
Use an inner object instead of captured vars.
https://codereview.appspot.com/7547045
Minimal diff for converting generator interface wrap to a function.
https://codereview.appspot.com/7547046
Use RuntimeInliner to provide the generator common code.
https://codereview.appspot.com/7487046

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/YieldState.js
File src/codegeneration/generator/YieldState.js (right):

https://codereview.appspot.com/7547045/diff/1/src/codegeneration/generator/YieldState.js#newcode64
src/codegeneration/generator/YieldState.js:64: // this.$current =
expression;

On 2013/03/13 22:25:18, arv-chromium wrote:
> What is this here? I guess I should apply the patch and see what is
generated.

It's the inner 'G' object.

https://codereview.appspot.com/7547045/diff/5001/src/codegeneration/generator/GeneratorTransformer.js
File src/codegeneration/generator/GeneratorTransformer.js (left):

https://codereview.appspot.com/7547045/diff/5001/src/codegeneration/generator/GeneratorTransformer.js#oldcode326
src/codegeneration/generator/GeneratorTransformer.js:326:
createAssignStateStatement(machineEndState),
Strictly speaking, this isn't necessary either, but removing this makes
the state machine state incomplete unless you also refer to outer
GState.

https://codereview.appspot.com/7547045/diff/5001/src/codegeneration/generator/GeneratorTransformer.js
File src/codegeneration/generator/GeneratorTransformer.js (right):

https://codereview.appspot.com/7547045/diff/5001/src/codegeneration/generator/GeneratorTransformer.js#newcode220
src/codegeneration/generator/GeneratorTransformer.js:220: GState:
${ST_NEWBORN},
We have '$state' already, and there are a few places that would just
have 'this.state' without 'GState', so retained the 'G' for clarity.
Overly paranoid?

Description:
Use an inner object instead of captured vars.

- This will facilitate factoring out common generator code into a
function later.
- Also, remove redundant GSTATE assignment in machineEndStatements.

BUG=None
TEST=None


Please review this at https://codereview.appspot.com/7547045/

Affected files:
M src/codegeneration/generator/GeneratorTransformer.js
M src/codegeneration/generator/ReturnState.js
M src/codegeneration/generator/YieldState.js
M src/syntax/PredefinedName.js


Index: src/codegeneration/generator/GeneratorTransformer.js
diff --git a/src/codegeneration/generator/GeneratorTransformer.js
b/src/codegeneration/generator/GeneratorTransformer.js
index
5c75a63be4d7cb8072e4b9342ca1c6194edb105d..e0100215a0a825049fa44b53d25a059560af8065
100644
--- a/src/codegeneration/generator/GeneratorTransformer.js
+++ b/src/codegeneration/generator/GeneratorTransformer.js
@@ -51,6 +51,7 @@ import {
createPropertyNameAssignment,
createReturnStatement,
createStatementList,
+ createThisExpression,
createThrowStatement,
createUndefinedExpression,
createVariableStatement
@@ -64,8 +65,7 @@ var ST_NEWBORN = 0;
var ST_EXECUTING = 1;
var ST_SUSPENDED = 2;
var ST_CLOSED = 3;
-var GSTATE = '$GState';
-var $GSTATE = createIdentifierExpression(GSTATE);
+var GSTATE = 'GState';
var $YIELD_RETURN = createIdentifierExpression(YIELD_RETURN);

/**
@@ -196,8 +196,7 @@ export class GeneratorTransformer extends
CPSTransformer {

var statements = [];

- var $MOVE_NEXT = createIdentifierExpression(MOVE_NEXT);
- var $CURRENT = createIdentifierExpression(CURRENT);
+ var $G = createIdentifierExpression('$G');

// TODO(arv): Simplify the outputted code by only alpha renaming this
and
// arguments if needed.
@@ -217,11 +216,12 @@ export class GeneratorTransformer extends
CPSTransformer {

statements.push(
parseStatement `
- var
- ${$GSTATE} = ${ST_NEWBORN},
- ${$CURRENT},
- ${$YIELD_RETURN},
- ${$MOVE_NEXT} = ${this.generateMachineMethod(machine)};
+ var ${$G} = {
+ GState: ${ST_NEWBORN},
+ current: undefined,
+ yieldReturn: undefined,
+ moveNext: ${this.generateMachineMethod(machine)}
+ };
`);

// TODO(arv): The result should be an instance of Generator.
@@ -235,7 +235,7 @@ export class GeneratorTransformer extends
CPSTransformer {
parseStatement `
var $result = {
send: function(x) {
- switch (${$GSTATE}) {
+ switch (${$G}.GState) {
case ${ST_EXECUTING}:
throw new Error('"send" on executing generator');
case ${ST_CLOSED}:
@@ -246,14 +246,15 @@ export class GeneratorTransformer extends
CPSTransformer {
}
// fall through
case ${ST_SUSPENDED}:
- ${$GSTATE} = ${ST_EXECUTING};
- if (${$MOVE_NEXT}(x, ${ACTION_SEND})) {
- ${$GSTATE} = ${ST_SUSPENDED};
- return ${$CURRENT};
+ ${$G}.GState = ${ST_EXECUTING};
+ if (${$G}.moveNext(x, ${ACTION_SEND})) {
+ ${$G}.GState = ${ST_SUSPENDED};
+ return ${$G}.current;
}
- ${$GSTATE} = ${ST_CLOSED};
- if (${$YIELD_RETURN} !== undefined) {
- throw new
traceur.runtime.GeneratorReturn(${$YIELD_RETURN});
+ ${$G}.GState = ${ST_CLOSED};
+ if (${$G}.yieldReturn !== undefined) {
+ throw new traceur.runtime.
+ GeneratorReturn(${$G}.yieldReturn);
}
throw traceur.runtime.StopIteration;
}
@@ -264,41 +265,42 @@ export class GeneratorTransformer extends
CPSTransformer {
},

'throw': function(x) {
- switch (${$GSTATE}) {
+ switch (${$G}.GState) {
case ${ST_EXECUTING}:
throw new Error('"throw" on executing generator');
case ${ST_CLOSED}:
throw new Error('"throw" on closed generator');
case ${ST_NEWBORN}:
- ${$GSTATE} = ${ST_CLOSED};
+ ${$G}.GState = ${ST_CLOSED};
throw x;
case ${ST_SUSPENDED}:
- ${$GSTATE} = ${ST_EXECUTING};
- if (${$MOVE_NEXT}(x, ${ACTION_THROW})) {
- ${$GSTATE} = ${ST_SUSPENDED};
- return ${$CURRENT};
+ ${$G}.GState = ${ST_EXECUTING};
+ if (${$G}.moveNext(x, ${ACTION_THROW})) {
+ ${$G}.GState = ${ST_SUSPENDED};
+ return ${$G}.${CURRENT};
}
- ${$GSTATE} = ${ST_CLOSED};
- if (${$YIELD_RETURN} !== undefined) {
- throw new
traceur.runtime.GeneratorReturn(${$YIELD_RETURN});
+ ${$G}.GState = ${ST_CLOSED};
+ if (${$G}.yieldReturn !== undefined) {
+ throw new traceur.runtime.
+ GeneratorReturn(${$G}.yieldReturn);
}
throw traceur.runtime.StopIteration;
}
},

close: function() {
- switch (${$GSTATE}) {
+ switch (${$G}.GState) {
case ${ST_EXECUTING}:
throw new Error('"close" on executing generator');
case ${ST_CLOSED}:
return;
case ${ST_NEWBORN}:
- ${$GSTATE} = ${ST_CLOSED};
+ ${$G}.GState = ${ST_CLOSED};
return;
case ${ST_SUSPENDED}:
- ${$GSTATE} = ${ST_EXECUTING};
- ${$MOVE_NEXT}(undefined, ${ACTION_CLOSE});
- ${$GSTATE} = ${ST_CLOSED};
+ ${$G}.GState = ${ST_EXECUTING};
+ ${$G}.moveNext(undefined, ${ACTION_CLOSE});
+ ${$G}.GState = ${ST_CLOSED};
}
}
};`);
@@ -322,7 +324,9 @@ export class GeneratorTransformer extends
CPSTransformer {
*/
machineUncaughtExceptionStatements(rethrowState, machineEndState) {
return createStatementList(
- createAssignmentStatement($GSTATE, createNumberLiteral(ST_CLOSED)),
+ createAssignmentStatement(
+ createMemberExpression(createThisExpression(), GSTATE),
+ createNumberLiteral(ST_CLOSED)),
createAssignStateStatement(machineEndState),

createThrowStatement(createIdentifierExpression(STORED_EXCEPTION)));
}
@@ -346,9 +350,7 @@ export class GeneratorTransformer extends
CPSTransformer {

/** @return {Array.<ParseTree>} */
machineEndStatements() {
- return [
- createAssignmentStatement($GSTATE, createNumberLiteral(ST_CLOSED)),
- createReturnStatement(createFalseLiteral())];
+ return [createReturnStatement(createFalseLiteral())];
}
}

Index: src/codegeneration/generator/ReturnState.js
diff --git a/src/codegeneration/generator/ReturnState.js
b/src/codegeneration/generator/ReturnState.js
index
064531209f5bc81f2af84aa7256cdbd65036229d..4c73cfa818a5c06b85cd9db8f4f46f8c6faac73d
100644
--- a/src/codegeneration/generator/ReturnState.js
+++ b/src/codegeneration/generator/ReturnState.js
@@ -30,6 +30,7 @@ import {
createIdentifierExpression,
createMemberExpression,
createReturnStatement,
+ createThisExpression,
createTrueLiteral
} from '../ParseTreeFactory.js';

@@ -47,9 +48,13 @@ export class ReturnState extends YieldState {
var e = this.expression;
if (e && !isUndefined(e) && !isVoidExpression(e)) {
return [
- // $yieldReturn = expression;
+ // 'this' refers to the '$G' object from
+ // GeneratorTransformer.transformGeneratorBody
+ //
+ // this.$yieldReturn = expression;
createAssignmentStatement(
- createIdentifierExpression(YIELD_RETURN), this.expression),
+ createMemberExpression(createThisExpression(), YIELD_RETURN),
+ this.expression),
...State.generateJump(enclosingFinally, machineEndState)
];
} else {
Index: src/codegeneration/generator/YieldState.js
diff --git a/src/codegeneration/generator/YieldState.js
b/src/codegeneration/generator/YieldState.js
index
804172e9856e00bbe3e7aca201f950207f75acd5..7888cab3bcf910f2368140edede93d12f534639d
100644
--- a/src/codegeneration/generator/YieldState.js
+++ b/src/codegeneration/generator/YieldState.js
@@ -22,6 +22,7 @@ import {
createIdentifierExpression,
createMemberExpression,
createReturnStatement,
+ createThisExpression,
createTrueLiteral
} from '../ParseTreeFactory.js';

@@ -60,9 +61,13 @@ export class YieldState extends State {
*/
transform(enclosingFinally, machineEndState, reporter) {
return [
- // $current = expression;
+ // 'this' refers to the '$G' object from
+ // GeneratorTransformer.transformGeneratorBody
+ //
+ // this.$current = expression;
createAssignmentStatement(
- createIdentifierExpression(CURRENT), this.expression),
+ createMemberExpression(createThisExpression(), CURRENT),
+ this.expression),
// either:
// $state = this.fallThroughState;
// return true;
Index: src/syntax/PredefinedName.js
diff --git a/src/syntax/PredefinedName.js b/src/syntax/PredefinedName.js
index
be06b25f3194c69b88e11a34c84d2848ebde1124..d74adb4259e2f7bd6844a204ea834869144ea310
100644
--- a/src/syntax/PredefinedName.js
+++ b/src/syntax/PredefinedName.js
@@ -39,7 +39,7 @@ export var CREATE_CLASS = 'createClass';
export var CREATE_ERRBACK = '$createErrback';
export var CREATE_NAME = 'createName';
export var CREATE_PROMISE = 'createPromise';
-export var CURRENT = '$current';
+export var CURRENT = 'current';
export var DEFERRED = 'Deferred';
export var DEFINE_PROPERTIES = 'defineProperties';
export var DEFINE_PROPERTY = 'defineProperty';
@@ -67,7 +67,7 @@ export var ITERATOR = 'iterator';
export var LENGTH = 'length';
export var MODULE = 'module';
export var MODULES = 'modules';
-export var MOVE_NEXT = '$moveNext';
+export var MOVE_NEXT = 'moveNext';
export var NEW = 'new';
export var NEW_STATE = '$newState';
export var NUMBER = 'number';
@@ -106,7 +106,7 @@ export var VALUE = 'value';
export var WAIT_TASK = '$waitTask';
export var WRITABLE = 'writable';
export var YIELD_ACTION = '$yieldAction';
-export var YIELD_RETURN = '$yieldReturn';
+export var YIELD_RETURN = 'yieldReturn';
export var YIELD_SENT = '$yieldSent';
export function getParameterName(index) {
// TODO: consider caching these

[a...@chromium.org]

LGTM

https://codereview.appspot.com/7547045/

[a...@chromium.org]

Seems like all of them are LGTM'ed now... I'll try to commit them

https://codereview.appspot.com/7547045/