First round of comments, mostly style nits. The juicy bits are the
handling of
scopes in the fixup code, and some suggestions for tests to exercise
interaction
with other fancy scoping features.
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1273
src/ast.h:1273: // Delegates to another statement, which may be
overwritten.
Nit: class comments should go right above the declaration.
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1280
src/ast.h:1280: Statement* statement() { return statement_; }
Make this a const method
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1282
src/ast.h:1282: Scope* scope() { return scope_; }
ditto: const please
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1284
src/ast.h:1284: protected:
I realize the use of "protected" here matches some other cases, but it
makes no sense to me, since this is a "final" class. Can this just be
private?
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1285
src/ast.h:1285: explicit DelegateStatement(Zone* zone, int pos,
Statement* statement,
No need for "explicit" here.
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1287
src/ast.h:1287: : Statement(zone, pos), statement_(statement),
scope_(scope) {}
Could you just pass RelocInfo::kNoPosition to the base class here and
avoid accepting a |pos| argument? I can't imagine a use-case for a
DelegateStatement that actually has a position.
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4954
src/parser.cc:4954: void
Parser::InsertSloppyBlockFunctionVarBindings(Scope* scope, bool* ok) {
Can you add a DCHECK that the passed-in scope is a declaration scope?
Also, how does this interact with the case where a function has two var
declaration scopes (as it does for sloppy non-simple argument lists)?
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4958
src/parser.cc:4958: for (ZoneHashMap::Entry* p = map->Start(); p !=
NULL; p = map->Next(p)) {
Nit: I think "nullptr" is preferred these days over "NULL"
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4959
src/parser.cc:4959: AstRawString* name =
reinterpret_cast<AstRawString*>(p->key);
Can this just be a static_cast? Also, please re-constify it:
static_cast<const AstRawString*>(p->key)
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4962
src/parser.cc:4962: if (var == NULL ||
!IsLexicalVariableMode(var->mode())) {
s/NULL/nullptr/
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4967
src/parser.cc:4967: Declare(declaration, DeclarationDescriptor::NORMAL,
true, ok);
I'm a little bit confused: why do you use the passed-in scope above, but
the current scope_ here (since you don't pass the last argument)? Are
|scope| and |scope_| actually the same here?
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4973
src/parser.cc:4973:
reinterpret_cast<ZoneVector<DelegateStatement*>*>(p->value);
Same here, please static_cast instead of reinterpret_cast
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto stmt = delegates->begin(); stmt <
delegates->end(); stmt++) {
You can use more C++11 here:
for (auto stmt : delegates)
That'll also make stmt actually a Statement*, instead of an iterator.
Also, "delegate" is probably a better name for the loop var, since
you're already using "statement" below.
https://codereview.chromium.org/1332873003/diff/80001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1332873003/diff/80001/src/parser.h#newcode1137
src/parser.h:1137: void InsertSloppyBlockFunctionVarBindings(Scope*
scope, bool* ok);
I think this deserves a quick comment, perhaps just a reference to the
spec.
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode79
src/scopes.cc:79: // FIXME(marja): fix the type of Lookup.
What's this TODO about? The return value seems fine to me.
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode83
src/scopes.cc:83: if (!p->value) {
Nit: p->value == nullptr is the v8 style here, generally.
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode84
src/scopes.cc:84: p->value = new
(zone_->New(sizeof(ZoneVector<DelegateStatement*>)))
Might be worth typedefing ZoneVector<DelegateStatement*> to something as
you have to refer to it a bunch in this method.
https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/block-sloppy-function.js
File test/mjsunit/harmony/block-sloppy-function.js (right):
https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/block-sloppy-function.js#newcode5
test/mjsunit/harmony/block-sloppy-function.js:5: // Flags:
--no-legacy-const --harmony-sloppy --harmony-sloppy-let
--harmony-sloppy-function
You can have multiple "Flags:" lines to keep this under 80 chars
https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/block-sloppy-function.js#newcode7
test/mjsunit/harmony/block-sloppy-function.js:7: // Test Annex B 3.3
semantics for functions declared in blocks in sloppy mode.
Can you add tests for shadowing function params, both for simple
parameter lists and complex? I'm not actually sure what'll happen with
destructured param names at the moment, given that they're LET-bound,
but I'd like to know.
Also would be interesting to see this shadow "arguments".
https://codereview.chromium.org/1332873003/diff/80001/test/mjsunit/harmony/block-sloppy-function.js#newcode147
test/mjsunit/harmony/block-sloppy-function.js:147: // Test that hoisting
from blocks doesn't happen in global scope
How is this effect achieved?
https://codereview.chromium.org/1332873003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
