Mainly minor comments, looking good.
https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc
File src/a64/builtins-a64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc#newcode583
src/a64/builtins-a64.cc:583: __ bind(&count_incremented);
bind outside if?
https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc#newcode5388
src/a64/code-stubs-a64.cc:5388: // We should either have undefined in
ebx or a valid AllocationSite
x10
https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc#newcode498
src/arm/builtins-arm.cc:498: }
Is it possible to keep
__ add(r0, r4, Operand(r3, LSL, kPointerSizeLog2)); // End of object.
__ InitializeFieldsWithFiller(r5, r0, r6);
https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc#newcode635
src/arm/builtins-arm.cc:635: }
bind outside if?
https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc#newcode5276
src/arm/code-stubs-arm.cc:5276: // We should either have undefined in
ebx or a valid AllocationSite
r2
https://codereview.chromium.org/132963012/diff/210001/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/factory.cc#newcode1318
src/factory.cc:1318:
isolate()->heap()->AllocateJSObjectWithAllocationSite(
The refactoring of AllocateJSObject* should happen in a differen CL. I
think we should keep separate factory methods.
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc#newcode8224
src/hydrogen.cc:8224: if (!allocation_site.is_null()) {
When is the allocation_site null?
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h#newcode1002
src/hydrogen.h:1002: : current_site_(NULL), pretenure_flag_(NOT_TENURED)
{}
indent
https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32.cc#newcode268
src/ia32/builtins-ia32.cc:268: __ InitializeFieldsWithFiller(ecx, edi,
edx);
Should we keep this one out of the if/else construct, the way it was
before?
https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32.cc#newcode399
src/ia32/builtins-ia32.cc:399: __ bind(&count_incremented);
Can you move the bind outside the if case?
https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc#newcode8304
src/runtime.cc:8304: // TODO(mvstanton): make this a real site
ambiguous comment
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc#newcode272
src/x64/builtins-x64.cc:272: }
__ InitializeFieldsWithFiller(rcx, rdi, rdx); outside the if/else?
https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc#newcode404
src/x64/builtins-x64.cc:404: }
__ bind(&count_incremented); outside of if?
https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):
https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler-x64.cc#newcode4052
src/x64/macro-assembler-x64.cc:4052: bind(&fine);
I don't think this code is needed...
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc#newcode2533
test/cctest/test-heap.cc:2533: " return this;"
remove return this;
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc#newcode2579
test/cctest/test-heap.cc:2579: // Test that a form of call-new
pretenuring works even without allocation sites.
Test global pretenuring of call new.
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mementos.cc
File test/cctest/test-mementos.cc (right):
https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mementos.cc#newcode86
test/cctest/test-mementos.cc:86: "a;";
a == return a; ?
https://codereview.chromium.org/132963012/
--
--
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/groups/opt_out.
