lgtm
https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#newcode2516
src/arm/full-codegen-arm.cc:2516: __ push(scratch);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Add a comment about why it is ok to skip access check here
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#newcode2525
src/arm/full-codegen-arm.cc:2525: __ ldr(ip, MemOperand(sp,
kPointerSize)); // constructor
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: use scratch register instead of 'ip' here
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#newcode2572
src/arm/full-codegen-arm.cc:2572: context()->Plug(r0);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> The caller plugs context, no need to do it here.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc
File src/arm64/full-codegen-arm64.cc (right):
https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc#newcode2188
src/arm64/full-codegen-arm64.cc:2188: __ Push(scratch);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Comment on why access check is not needed.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc#newcode2197
src/arm64/full-codegen-arm64.cc:2197: __ Peek(x1, kPointerSize); //
constructor
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: Use scratch register here.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc#newcode2244
src/arm64/full-codegen-arm64.cc:2244: context()->Plug(x0);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> The caller plugs context, no need to do this here.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h
File src/full-codegen.h (right):
https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h#newcode567
src/full-codegen.h:567: // Expects the class (function) in the
accumulator.
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: add a comment that accumulator contains constructor after the
function
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):
https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc#newcode2430
src/ia32/full-codegen-ia32.cc:2430: __ mov(scratch, FieldOperand(eax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Access check comment.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc#newcode2481
src/ia32/full-codegen-ia32.cc:2481: context()->Plug(eax);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Caller plugs context; no need to do it here
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#newcode2428
src/x64/full-codegen-x64.cc:2428: Register scratch = rcx;
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Nit: Why rcx here, but ebx in ia32?
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#newcode2429
src/x64/full-codegen-x64.cc:2429: __ movp(scratch, FieldOperand(rax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Access check comment.
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#newcode2480
src/x64/full-codegen-x64.cc:2480: context()->Plug(rax);
On 2014/10/28 09:42:00, arv wrote:
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Caller plugs context
Done.
Acknowledged.
https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):
https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes.js#newcode180
test/mjsunit/harmony/classes.js:180:
On 2014/10/27 22:20:53, arv wrote:
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Additional tests:
> 1. Test that 'DefineProperty' is not a store (extend a class that
has
> getter/setter, define method with same name)
I thought I tested that but I guess my tests where just manual
tests... I'll add
tests.
> 2. Test property descriptors of things that are not __proto__.
Will add.
> 3. Tests with super calls (do I understand correctly though that
these do not
> work yet since home object is not installed on methods?)
I was planning on adding that in a follow up CL. I'll expand the CL
descriptions.
Acknowledged.
https://codereview.chromium.org/680993003/
--
--
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.
