A few small comments.
I think when you look up a variable in an outer scope based on a serialized
scope info, and find it present in the context, you need to add it to the
variable map for that scope rather than allocating a fresh one.
I'm not entirely satisfied with patching up the scope chain after parsing
and
before scope analysis. It still seems cleaner that we pass a correct scope
chain into the parser for lazy parsing. Maybe we should make that a goal
for
long-term cleanup of this part of the code.
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc#newcode1230
src/hydrogen-instructions.cc:1230: stream->Add("[%d->%d]]",
context_chain_length(), slot_index());
I think you have an extra "]" here. I'm not wild about the ASCII arrow,
because it doesn't really signify anything. "(%d,%d) seems fine to me.
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
This predicate (->AsSlot() != NULL && ->AsSlot()->type == Slot::CONTEXT)
should probably be defined on the Variable class as something like
"IsInContext()" or "IsHeapAllocated()".
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2965
src/hydrogen.cc:2965: BAILOUT("reference to
non-stack-allocated/non-global variable");
You should probably change this bailout string. All that's left is
Slot::LOOKUP, right?
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3463
src/hydrogen.cc:3463: if (var->AsSlot() != NULL && var->AsSlot()->type()
== Slot::CONTEXT) {
Instead of an early bailout, it would be better if this code should be
structured like the code in VisitVariableProxy, explicitly handling the
cases:
if (/* global */) ...
else if (/* stack-allocated */) ...
else /* the rest, bailout */
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: } else {
Because here we don't want to have Slot::LOOKUP fall through the cracks.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc#newcode1823
src/ia32/lithium-codegen-ia32.cc:1823: HLoadContextSlot* hinstr =
instr->hydrogen();
It's typical for the Lithium instructions to have accessors like
slot_index and context_chain_length that just forward to the hydrogen
instruction.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc#newcode1824
src/ia32/lithium-codegen-ia32.cc:1824: Comment(";;; Load context slot %d
at %d",
There will automatically be a comment with the printed representation of
the instruction without having to emit it explicitly.
You should just have to override the print data function for
LLoadContextSlot to print the height and offset.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc#newcode274
src/ia32/lithium-ia32.cc:274: hydrogen()->PrintDataTo(stream);
I wouldn't forward to the hydrogen instruction like this. There is no
guarantee that the printed representation of a hydrogen instruction
makes sense in a Lithium context.
http://codereview.chromium.org/5753005/diff/27001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/objects.cc#newcode5381
src/objects.cc:5381: || code() == shared()->code());
For some reason we like to put || on the first line. In any case, the
second line should be lined up under the "s" in shared() above it.
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#newcode139
src/runtime-profiler.cc:139: && function->shared()->code() == code;
Why is this last check necessary?
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#newcode165
src/runtime-profiler.cc:165: ||
!function->shared()->allows_lazy_compilation()) {
It makes more sense to move this check down below with the other checks
on the shared code (after SharedFunctionInfo* shared =
function->shared()')
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc
File src/scopes.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode133
src/scopes.cc:133: // At some point we might want to provide outer
scopes to
Keep this comment near the ASSERT it mentions.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode241
src/scopes.cc:241: return new Variable(this, name, Variable::VAR, true,
Variable::ARGUMENTS);
We don't want to return a fresh variable every time we lookup the same
symbol in the same scope. We lose the property that the same identifier
always resolves to the same variable in the same scope.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name,
&mode);
We also want to assert (at least) that we do not find this name as a
stack-allocated variable in this scope. If we do then something has
gone wrong.
http://codereview.chromium.org/5753005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
