https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc File src/full-codegen.cc (right):
https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc#newcode746 src/full-codegen.cc:746: __ push(Immediate(Smi::FromInt(interface->Index()))); On 2012/11/20 12:05:05, Michael Starzinger wrote:
See comment further down the file.
Done. https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc#newcode764 src/full-codegen.cc:764: // context in the host context. On 2012/11/20 14:39:13, Sven Panne wrote:
Can we abstract this index magic into a separate class instead of
re-using a
plain FixedArray? The comments should go there, if they are necessary
then at
all. Even publicly inheriting from FixedArray (which we do a lot)
might be a
win.
Done. Of course, in C++ that means thrice as much code. https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc#newcode822 src/full-codegen.cc:822: description->set(1, *scope->GetScopeInfo()); On 2012/11/20 12:05:05, Michael Starzinger wrote:
Pattern is not GC-safe.
Obsolete. https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc#newcode822 src/full-codegen.cc:822: description->set(1, *scope->GetScopeInfo()); On 2012/11/20 14:39:13, Sven Panne wrote:
See my comment above about FixedArray. The description seems to be a
sum type,
at least this layout is different from the one above. Anyway, this is
hard to
follow in its current form.
That was actually a bug (leftover from earlier format), the second field shouldn't have been there. https://codereview.chromium.org/11093074/diff/1/src/full-codegen.cc#newcode1123 src/full-codegen.cc:1123: __ push(Immediate(Smi::FromInt(stmt->proxy()->interface()->Index()))); On 2012/11/20 12:05:05, Michael Starzinger wrote:
You will have a hard time porting this to other architectures, because
the
Immediate operand cannot be used there. Better use the uppercase "__
Push()"
wrapper for that.
Done. https://codereview.chromium.org/11093074/diff/1/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): https://codereview.chromium.org/11093074/diff/1/src/ia32/full-codegen-ia32.cc#newcode895 src/ia32/full-codegen-ia32.cc:895: while (host->is_module_scope()) host = host->outer_scope(); On 2012/11/20 12:05:05, Michael Starzinger wrote:
Can we move this into a Scope::GlobalScope() helper? We already have a
similar
thing for Scope::DeclarationScope().
Done. https://codereview.chromium.org/11093074/diff/1/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/11093074/diff/1/src/runtime.cc#newcode1361 src/runtime.cc:1361: ASSERT(is_var + is_const + is_function == 1); On 2012/11/20 14:39:13, Sven Panne wrote:
Cunning check for mutual exclusion... o_O Anyway, I don't think we
need this
here, but I leave it up to you.
You think so? There actually was a subtle bug with that while I was working on some earlier CL, which is why I put it here. ;) https://codereview.chromium.org/11093074/diff/1/src/runtime.cc#newcode8508 src/runtime.cc:8508: while (previous->IsModuleContext()) previous = previous->previous(); On 2012/11/20 12:05:05, Michael Starzinger wrote:
Use a Context::global_context() helper here.
Done. https://codereview.chromium.org/11093074/diff/1/src/runtime.cc#newcode8517 src/runtime.cc:8517: static_cast<PropertyAttributes>(READ_ONLY | DONT_DELETE | DONT_ENUM); On 2012/11/20 12:05:05, Michael Starzinger wrote:
It might make sense to move these constants into the
PropertyAttributes class
itself, so that others can use them as well.
Done. https://codereview.chromium.org/11093074/diff/1/src/runtime.cc#newcode8517 src/runtime.cc:8517: static_cast<PropertyAttributes>(READ_ONLY | DONT_DELETE | DONT_ENUM); On 2012/11/20 14:39:13, Sven Panne wrote:
On 2012/11/20 12:05:05, Michael Starzinger wrote: > It might make sense to move these constants into the
PropertyAttributes class
> itself, so that others can use them as well.
... and it would make even more sense to clean up the confusion
between elements
of a set and a set itself, but that is a different story. :-)
Yeah... https://codereview.chromium.org/11093074/diff/1/src/runtime.cc#newcode8571 src/runtime.cc:8571: USE(JSObject::PreventExtensions(module)); On 2012/11/20 12:05:05, Michael Starzinger wrote:
There should be no need to use the return value of the handlified
version. Done. https://codereview.chromium.org/11093074/diff/1/test/cctest/test-decls.cc File test/cctest/test-decls.cc (right): https://codereview.chromium.org/11093074/diff/1/test/cctest/test-decls.cc#newcode737 test/cctest/test-decls.cc:737: On 2012/11/20 12:05:05, Michael Starzinger wrote:
Drop the third empty newline.
Done. https://codereview.chromium.org/11093074/diff/1/test/cctest/test-decls.cc#newcode779 test/cctest/test-decls.cc:779: On 2012/11/20 12:05:05, Michael Starzinger wrote:
Drop the third empty newline.
Done. https://codereview.chromium.org/11093074/diff/1/test/cctest/test-decls.cc#newcode808 test/cctest/test-decls.cc:808: On 2012/11/20 12:05:05, Michael Starzinger wrote:
Drop the third empty newline.
Done. https://codereview.chromium.org/11093074/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
