I assume we will never enable Harmony syntax in non-Harmony-opt-in code.
I.e.,
this is only ever going to be used for testing out the feature.
Will it live on a branch, or are we planning to merge this to
bleeding_edge/trunk?
I found myself wanting to try out the patch, but I couldn't find the
baseline
code. Is there a combined patch somewhere that can be used to run the code?
http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc
File preparser/preparser-process.cc (right):
http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc#newcode41
preparser/preparser-process.cc:41: } } // namespace v8::internal
Put this in preparser-api.cc.
That's where other "essential" parts of V8 are emulated.
Or, preferably, don't use FLAG_harmony_block_scoping in the scanner or
preparser, but pass a flag to the scanner/parser when they are created.
Better to inject than to depend on global variables. It reduces
dependencies.
http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):
http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
Should let be grouped with dynamic? If it occurs inside a loop, and the
scope also contains a function expression, it's possible to capture
multiple versions of the same variable (similar to a catch-scope). I.e.,
it can't be assigned to a single stack slot.
http://codereview.chromium.org/7616009/diff/1/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1108
src/parser.cc:1108: return ParseVariableStatement(true, ok);
Please use an enum with meaningful names for the first argument.
Generally try to avoid passing booleans as true/false (or other magical
constants).
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1535
src/parser.cc:1535: inside_with());
What baseline is this patch based on? I don't see any block-scope in the
current code?
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1553
src/parser.cc:1553: Statement* stat = ParseSourceElement(NULL,
CHECK_OK);
Will this enable function declarations inside blocks in strict mode?
It's allowed as a SourceElement but not as a Statement.
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3562
src/parser.cc:3562: next = Token::FUTURE_STRICT_RESERVED_WORD;
Why not just add a case for Token::LET next to
Token::FUTURE_STRICT_RESERVED_WORD?
Or not bother, and let it be handled like a keyword?
Both should work.
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3752
src/parser.cc:3752: !harmony_block_scoping_)
If harmony_block_scoping_ is set, function declarations are not hoisted?
http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode4055
src/parser.cc:4055: // Parses and identifier that is valid for the
current scope, in particular it
"and"->"an" while you are here :)
http://codereview.chromium.org/7616009/diff/1/src/preparser.cc
File src/preparser.cc (right):
http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode89
src/preparser.cc:89: // fallthrough
No need to write "fallthrough" if the two case labels are adjacent. It's
only if there is some other code in the first one that it's necessary.
http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode125
src/preparser.cc:125: }
Need fallthrough here. Or drop the switch and just do:
if (peek()== i::Token::LET && harmony_block_scoping_) {
return ParseVariabelStatement(ACCEPT_LET, ok);
}
return ParseStatement(ok);
http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode344
src/preparser.cc:344: // is the entire function (ECMA-262, 3rd, 10.1.3,
and 12.2). .
This comment seems to be outdated now.
And it has an extra '.' at the end :)
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scoping.js
File test/mjsunit/harmony/block-scoping.js (right):
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scoping.js#newcode58
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
Agree. There is no test here that can distinguish a let from a hoisted
var.
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scoping.js#newcode102
test/mjsunit/harmony/block-scoping.js:102: }
Try creating let variables in a loop. E.g.:
var accessors = [];
for (var i = 0; i < 10; i++) {
let x = i;
accessors[i] = function() { return x; }
}
for (var i = 0; i < 10; i++) {
assertEquals(i, accessors[i]());
}
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-blockscopes.js
File test/mjsunit/harmony/debug-blockscopes.js (right):
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-blockscopes.js#newcode387
test/mjsunit/harmony/debug-blockscopes.js:387: //
CheckScopeContent({z:4}, 1, exec_state);
Remove the TODO and commented code.
If it's not working, file a bug for it and put a test in mjsunit/bugs
(those are assumed to fail).
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-evaluate-blockscopes.js
File test/mjsunit/harmony/debug-evaluate-blockscopes.js (right):
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-evaluate-blockscopes.js#newcode28
test/mjsunit/harmony/debug-evaluate-blockscopes.js:28: // Flags:
--expose-debug-as debug
Needs harmony-block-scoping flag?
Does it run without? And if so, is something wrong?
http://codereview.chromium.org/7616009/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
