LGTM with comments (mostly nits, but a few real issues too).
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 nit: += "map." 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. This comment seems misplaced. http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode769 src/builtins.cc:769: int result_len = final - k; "= Max(final - k, 0)" please, as there's no guarantee that the difference is >=0. http://codereview.chromium.org/9073007/diff/26027/src/builtins.cc#newcode979 src/builtins.cc:979: if (!JSArray::cast(arg)->HasFastTypeElements()) { 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()) { 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); nit: can fit in one line http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3740 src/heap.cc:3740: ASSERT(length == 0); nit: unnecessary, covered by ASSERT(capacity >= length) above. http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3751 src/heap.cc:3751: if (!maybe_elms->To(&elms)) return maybe_elms; This line occurs four times. You could move it out of the if-blocks, then you'd need it only once. http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode3784 src/heap.cc:3784: pretenure); nit: can fit in one line http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4511 src/heap.cc:4511: reinterpret_cast<FixedDoubleArray*>(elements_object); 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; http://codereview.chromium.org/9073007/diff/26027/src/heap.cc#newcode4525 src/heap.cc:4525: MaybeObject* maybe_obj = AllocateRawFixedDoubleArray(length, pretenure); 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 nit: s/ / / http://codereview.chromium.org/9073007/diff/26027/src/heap.h#newcode2658 src/heap.h:2658: nit: No reason for an empty line here. 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 nit: += "map." 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()) { What about the current_map == global_context->double_js_array_map() case? Not frequent enough to be handled here? http://codereview.chromium.org/9073007/diff/26027/src/objects-inl.h#newcode3920 src/objects-inl.h:3920: maybe_map = initial_map->CopyDropTransitions(); I'd s/initial_map/new_double_map/ here to make it more obvious what's going on. 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) { nit: indentation was correct before 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, nit: could put all three args onto the same line. Up to you. 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. nit: superfluous "j" (does this compile?) 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 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." 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 nit: += "map." 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. nit: 2012 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. nit: 2012 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. nit: 2012 http://codereview.chromium.org/9073007/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
