Haven't figured out tests for this scoping issue, but fixed all the other
review
comments
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.
On 2015/09/11 at 15:42:56, adamk wrote:
Nit: class comments should go right above the declaration.
Done
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1280
src/ast.h:1280: Statement* statement() { return statement_; }
On 2015/09/11 at 15:42:56, adamk wrote:
Make this a const method
Done
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1282
src/ast.h:1282: Scope* scope() { return scope_; }
On 2015/09/11 at 15:42:56, adamk wrote:
ditto: const please
Done
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1284
src/ast.h:1284: protected:
On 2015/09/11 at 15:42:56, adamk wrote:
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?
Done
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1285
src/ast.h:1285: explicit DelegateStatement(Zone* zone, int pos,
Statement* statement,
On 2015/09/11 at 15:42:56, adamk wrote:
No need for "explicit" here.
Done
https://codereview.chromium.org/1332873003/diff/80001/src/ast.h#newcode1287
src/ast.h:1287: : Statement(zone, pos), statement_(statement),
scope_(scope) {}
On 2015/09/11 at 15:42:56, adamk wrote:
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.
Done
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) {
On 2015/09/11 at 15:42:56, adamk wrote:
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)?
Done
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)) {
On 2015/09/11 at 15:42:56, adamk wrote:
Nit: I think "nullptr" is preferred these days over "NULL"
Done
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4959
src/parser.cc:4959: AstRawString* name =
reinterpret_cast<AstRawString*>(p->key);
On 2015/09/11 at 15:42:56, adamk wrote:
Can this just be a static_cast? Also, please re-constify it:
static_cast<const AstRawString*>(p->key)
Done
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4962
src/parser.cc:4962: if (var == NULL ||
!IsLexicalVariableMode(var->mode())) {
On 2015/09/11 at 15:42:56, adamk wrote:
s/NULL/nullptr/
Done
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4967
src/parser.cc:4967: Declare(declaration, DeclarationDescriptor::NORMAL,
true, ok);
On 2015/09/11 at 15:42:56, adamk wrote:
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?
It happens to work because this is called when coming back out to the
function scope, but that's unclear, so I passed scope.
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4973
src/parser.cc:4973:
reinterpret_cast<ZoneVector<DelegateStatement*>*>(p->value);
On 2015/09/11 at 15:42:56, adamk wrote:
Same here, please static_cast instead of reinterpret_cast
Done
https://codereview.chromium.org/1332873003/diff/80001/src/parser.cc#newcode4974
src/parser.cc:4974: for (auto stmt = delegates->begin(); stmt <
delegates->end(); stmt++) {
On 2015/09/11 at 15:42:56, adamk wrote:
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.
Done
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);
On 2015/09/11 at 15:42:56, adamk wrote:
I think this deserves a quick comment, perhaps just a reference to the
spec.
Done
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.
On 2015/09/11 at 15:42:56, adamk wrote:
What's this TODO about? The return value seems fine to me.
I think it's about using the hash. I copied it from the other map.
Probably this is not so useful; deleted the TODO.
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode83
src/scopes.cc:83: if (!p->value) {
On 2015/09/11 at 15:42:56, adamk wrote:
Nit: p->value == nullptr is the v8 style here, generally.
Done
https://codereview.chromium.org/1332873003/diff/80001/src/scopes.cc#newcode84
src/scopes.cc:84: p->value = new
(zone_->New(sizeof(ZoneVector<DelegateStatement*>)))
On 2015/09/11 at 15:42:56, adamk wrote:
Might be worth typedefing ZoneVector<DelegateStatement*> to something
as you have to refer to it a bunch in this method.
Done
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
On 2015/09/11 at 15:42:56, adamk wrote:
You can have multiple "Flags:" lines to keep this under 80 chars
Done
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
On 2015/09/11 at 15:42:56, adamk wrote:
How is this effect achieved?
By 'global' I meant 'not in a function'. This is tested because we see
that the local globalUnhoisted isn't hoisted, and we reach the outer
one, because its return value is 0 not 1.
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.
