feedback addressed, landing
http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/arm/macro-assembler-arm.cc#newcode2899
src/arm/macro-assembler-arm.cc:2899: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
nit: += "map."
Done.
http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc
File src/builtins.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode200
src/builtins.cc:200: // Initialize elements and length in case the
transition fails.
On 2012/01/23 17:16:55, Jakob wrote:
This comment seems misplaced.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode769
src/builtins.cc:769: int result_len = final - k;
On 2012/01/23 17:16:55, Jakob wrote:
"= Max(final - k, 0)" please, as there's no guarantee that the
difference is
>=0.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode979
src/builtins.cc:979: if (!JSArray::cast(arg)->HasFastTypeElements()) {
On 2012/01/23 17:16:55, Jakob wrote:
This condition will never be true, as it's already checked for (and
bailed out)
above (line 961). I think you mean:
if (JSArray::cast(arg)->HasFastElements()) {
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3735
src/heap.cc:3735: pretenure);
On 2012/01/23 17:16:55, Jakob wrote:
nit: can fit in one line
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3740
src/heap.cc:3740: ASSERT(length == 0);
On 2012/01/23 17:16:55, Jakob wrote:
nit: unnecessary, covered by ASSERT(capacity >= length) above.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3751
src/heap.cc:3751: if (!maybe_elms->To(&elms)) return maybe_elms;
On 2012/01/23 17:16:55, Jakob wrote:
This line occurs four times. You could move it out of the if-blocks,
then you'd
need it only once.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3784
src/heap.cc:3784: pretenure);
On 2012/01/23 17:16:55, Jakob wrote:
nit: can fit in one line
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4511
src/heap.cc:4511: reinterpret_cast<FixedDoubleArray*>(elements_object);
Unfortunately, you can't use FixedDoubleArray::cast here because the
object isn't well formed yet (it has no map).
On 2012/01/23 17:16:55, Jakob wrote:
While you're at it, you could replace these five lines with the
following three:
FixedDoubleArray* elements;
MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length,
pretenure);
if (!maybe_obj->To(&elements)) return maybe_obj;
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4525
src/heap.cc:4525: MaybeObject* maybe_obj =
AllocateRawFixedDoubleArray(length, pretenure);
Same here.
On 2012/01/23 17:16:55, Jakob wrote:
Same suggestion here: three lines is better than five.
Also, if you s/Raw/Uninitialized/, you don't need to set map and
length below.
http://codereview.chromium.org/9073007/diff/26027/src/heap.h
File src/heap.h (right):
http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode810
src/heap.h:810: // Allocates a fixed double array with hole values.
Returns
On 2012/01/23 17:16:55, Jakob wrote:
nit: s/ / /
Done.
http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode2658
src/heap.h:2658:
On 2012/01/23 17:16:55, Jakob wrote:
nit: No reason for an empty line here.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/ia32/macro-assembler-ia32.cc#newcode2188
src/ia32/macro-assembler-ia32.cc:2188: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
nit: += "map."
Done.
http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode1301
src/objects-inl.h:1301: if (current_map ==
global_context->smi_js_array_map()) {
On 2012/01/23 17:16:55, Jakob wrote:
What about the current_map == global_context->double_js_array_map()
case? Not
frequent enough to be handled here?
The SMI -> Object transition is so frequent and otherwise lightweight
that it makes sure to handle it here. Transitions involving double are
less frequent.
http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode3920
src/objects-inl.h:3920: maybe_map = initial_map->CopyDropTransitions();
On 2012/01/23 17:16:55, Jakob wrote:
I'd s/initial_map/new_double_map/ here to make it more obvious what's
going on.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/objects.cc#newcode9666
src/objects.cc:9666: ElementsKind to_kind) {
On 2012/01/23 17:16:55, Jakob wrote:
nit: indentation was correct before
Done.
http://codereview.chromium.org/9073007/diff/26027/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/parser.cc#newcode4686
src/parser.cc:4686: elements,
On 2012/01/23 17:16:55, Jakob wrote:
nit: could put all three args onto the same line. Up to you.
Done.
http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode1
src/runtime.cc:1: j// Copyright 2012 the V8 project authors. All rights
reserved.
On 2012/01/23 17:16:55, Jakob wrote:
nit: superfluous "j" (does this compile?)
Done.
http://codereview.chromium.org/9073007/diff/26027/src/runtime.cc#newcode518
src/runtime.cc:518: // If the ElementsKind of the constant values of the
array literal are less
On 2012/01/23 17:16:55, Jakob wrote:
This comment is confusing. Suggestion:
"Ensure that the boilerplate object has FAST_ELEMENTS, unless the flag
is on or
the object is larger than the threshold."
Done.
http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):
http://codereview.chromium.org/9073007/diff/26027/src/x64/macro-assembler-x64.cc#newcode4054
src/x64/macro-assembler-x64.cc:4054: // Use the transitioned cached
On 2012/01/23 17:16:55, Jakob wrote:
nit: += "map."
Done.
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construct-transition.js
File test/mjsunit/array-construct-transition.js (right):
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/array-construct-transition.js#newcode1
test/mjsunit/array-construct-transition.js:1: // Copyright 2011 the V8
project authors. All rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind.js
File test/mjsunit/elements-kind.js (right):
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-kind.js#newcode1
test/mjsunit/elements-kind.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-transition.js
File test/mjsunit/elements-transition.js (right):
http://codereview.chromium.org/9073007/diff/26027/test/mjsunit/elements-transition.js#newcode1
test/mjsunit/elements-transition.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2012/01/23 17:16:55, Jakob wrote:
nit: 2012
Done.
http://codereview.chromium.org/9073007/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
