thanks for having a look
https://codereview.chromium.org/943543002/diff/180001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/943543002/diff/180001/src/ast.h#newcode1658
src/ast.h:1658: VariableProxy(Zone* zone, Variable* var, int
start_position,
On 2015/02/23 13:45:26, rossberg wrote:
You could pass a Location instead of the individual values.
As discussed offline: Didn't do this change because Location is an inner
class of Scanner, and making ast.h include scanner.h makes no sense.
Location ofc shouldn't be an inner class of Scanner, but I won't fix
that in this CL.
https://codereview.chromium.org/943543002/diff/180001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/943543002/diff/180001/src/parser.cc#newcode3753
src/parser.cc:3753: Variable::FUNCTION, kCreatedInitialized,
kNotAssigned);
On 2015/02/23 13:45:26, rossberg wrote:
Note that this is not a function declaration, but a local binding that
is always
initialised immediately. I don't think it requires special treatment
as a
function.
Done, in addition did the same change in Scope::LookupFunctionVar, I
don't think we need that to be Variable::FUNCTION either (but I'm not
100% sure about that).
https://codereview.chromium.org/943543002/diff/180001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/943543002/diff/180001/src/scopes.cc#newcode1078
src/scopes.cc:1078: // they are inside the same eval scope or different
ones.
On 2015/02/23 13:45:26, rossberg wrote:
Can you add a TODO regarding different evals? I suppose each direct
eval scope
somehow would need to know its call position in the outer program
text... Hm,
tricky... I think we want to disallow direct eval in strong mode
anyway, but
that still leaves toplevel bindings.
Added a todo.
So one error case is:
This throws:
"use strong"; eval("let x = 1;"); eval("x;")
But this doesn't:
"use strong"; eval("let x = 1;"); eval("if (false) { x; }");
But won't we need the context analysis for that anyway?
https://codereview.chromium.org/943543002/diff/180001/src/scopes.cc#newcode1084
src/scopes.cc:1084: proxy->position() < var->position() &&
!var->is_function() &&
On 2015/02/23 13:45:26, rossberg wrote:
I don't believe this is good enough. Consider:
let x = x+1
Here, proxy->pos > var->pos, but it's an error nonetheless.
We already have var->initializer_position, which marks the position
where a var
is known to be bound (and is used by codegen to eliminate unnecessary
hole
checks for lexical bindings, see e.g. full-codegen-ia32.cc:1474). In
the above
case that would point at the _end_ of the binding's RHS (but before
the next
binding, if there are multiple comma-separated ones).
However, you probably need to set that position properly for a couple
more
declaration forms now (previously it only mattered for lexical ones).
Also, I
just saw that it seems wrong in ParseClassDeclaration.
Fixed this to use initializer_position(), undid the position addition to
Variable, fixed a couple of places where the initializer position was
set, added test cases.
https://codereview.chromium.org/943543002/diff/180001/src/variables.h
File src/variables.h (right):
https://codereview.chromium.org/943543002/diff/180001/src/variables.h#newcode21
src/variables.h:21: enum Kind { NORMAL, FUNCTION, THIS, NEW_TARGET,
ARGUMENTS };
On 2015/02/23 14:45:55, arv wrote:
I find this change strange. How is FUNCTION related to the other ones?
The previous NORMAL is split into NORMAL and FUNCTION, to distinguish
declarations like:
let x = 0; // NORMAL
from
function x() { } // FUNCTION
In addition:
let x = function() { ..} // NORMAL
This is needed so that we can make this work:
function foo() {
bar; // ok
bar2; // not ok
}
function bar() { }
let bar2 = 0;
... it's not necessary that this information is carried in the Kind; but
it seemed suitable since nothing checked for == NORMAL (only == THIS, ==
ARGUMENTS), so splitting NORMAL was feasible, and, I don't fully
understand what exactly this "Kind" is, except intuitively "it's what
kind of a variable it is", and for that, function seemed a suitable
addition.
https://codereview.chromium.org/943543002/diff/180001/test/mjsunit/strong/declaration-after-use.js
File test/mjsunit/strong/declaration-after-use.js (right):
https://codereview.chromium.org/943543002/diff/180001/test/mjsunit/strong/declaration-after-use.js#newcode7
test/mjsunit/strong/declaration-after-use.js:7: (function
DeclarationAfterUse() {
On 2015/02/23 13:45:26, rossberg wrote:
More tests would be great, both positive and negative. In particular:
- different combinations of multi-bindings with uses on the RHS
- class declarations referring to themselves in different parts
- for loops
Added some tests here, probably not everything you had in mind.
https://codereview.chromium.org/943543002/
--
--
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.
