Can you PTAL again.
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
It's now injected into the scanner and preparser. I did not want to
change the public api of the preparser (i.e.
v8::preparser::PreParser::PreParseProgram) so the preparser reads the
flag from the scanner it gets.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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:
Can you elaborate why it should be grouped with dynamic?
If you mean your for-loop example:
var accessors = [];
for (var i = 0; i < 10; i++) {
let x = i;
accessors[i] = function() { return x; }
}
here x is a fresh variable for each iteration of the loop, i.e. the
accessor closures will point to different instances of it.
The semantics of
var accessors = [];
for (let i = 0; i < 10; i++) {
accessors[i] = function() { return i; }
}
though is now quite fixed yet. There is a choice if i is fresh for each
iteration or not.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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);
Done. See the VariableDeclarationContext enum. Also removed the
accept_IN from ParseVariableDeclarations.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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());
It is based on a CL that was still in review. It's committed now.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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);
The proposal allows local function declarations inside blocks, but they
shouldn't be allowed in a statement context that is not a source element
context. This function is not used in ES5 normal or strict mode.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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;
Now obsolete, since the scanner now produces the correct token based on
a flag.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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_)
Yes, function declarations are then also expected to be block scoped.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
"and"->"an" while you are here :)
Done.
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
The scanner is now producing the correct token based on a flag. The
default case is now used for let.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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: }
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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);
Done.
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). .
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
This comment seems to be outdated now.
And it has an extra '.' at the end :)
Done.
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.
Added your for-loop test that distinguishes those.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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#newcode58
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
On 2011/08/11 14:34:59, rossberg wrote:
It would be great if you could add tests for variable shadowing
(between args,
catch, and let, in various combinations).
Done.
http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scoping.js#newcode102
test/mjsunit/harmony/block-scoping.js:102: }
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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]());
}
Done.
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);
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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).
Done.
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
My fault. The flag is needed.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
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
