LGTM with nits.
http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#newcode1549 src/arm/full-codegen-arm.cc:1549: // Do tail-call to runtime routine. This is not a tail call. http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#newcode1553 src/arm/full-codegen-arm.cc:1553: // Array literal has ElementsKind of FAST_DOUBLE_ELEMENTS nit: missing dot at the end. http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#newcode1563 src/arm/full-codegen-arm.cc:1563: r7, Urgh. This wastes vertical space and does not increase readability IMHO. Suggestion: __ StoreNumberToDoubleElements( result_register(), r3, r6, r1, r4, r5, r9, r7, &slow_elements); Personally, I'm also fine with arbitrary groupings of parameters per line. http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.cc#newcode1528 src/ia32/full-codegen-ia32.cc:1528: // Do tail-call to runtime routine. This is not a tail call. http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.cc#newcode1532 src/ia32/full-codegen-ia32.cc:1532: // Array literal has ElementsKind of FAST_DOUBLE_ELEMENTS nit: missing dot at the end. http://codereview.chromium.org/8258015/diff/9001/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3315 src/parser.cc:3315: // ElementsKind. Start with FAST_SMI_ONLY_ELEMENTS, and transitions to nit: s/transitions/transition/ http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3331 src/parser.cc:3331: for (int j = 0; j < i; ++j) { This code works, but it's brittle, as it subtly relies on the "if (elements_kind == FAST_DOUBLE_ELEMENTS)" block being executed after the transition, which might bite anyone who tries to refactor this. Please do either of these two: (1) Put in an explicit store of boilerplate_value->Number into the newly allocated double array (and, at your option, insert an "else" below to avoid duplication of the store), or (2) Add a comment explaining the fall-through situation. http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode447 src/runtime.cc:447: // If the ElementKinds of the constant values of the array literal are less nit: s/ElementKinds/ElementsKind/, s/are/is/ http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode448 src/runtime.cc:448: // specific that the ElementsKind of the boilerplate array object, change the nit: s/that/than/ http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode452 src/runtime.cc:452: Handle<Map> smi_array_map = isolate->factory()->GetElementsTransitionMap( nit: rename smi_array_map to something more generic (transitioned_array_map? new_array_map?), as it might well be a non-smi map. http://codereview.chromium.org/8258015/diff/9001/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/x64/code-stubs-x64.cc#newcode291 src/x64/code-stubs-x64.cc:291: // Copy the elements array. Duplicate line. http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc File src/x64/full-codegen-x64.cc (right): http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#newcode1497 src/x64/full-codegen-x64.cc:1497: // Do tail-call to runtime routine. This is not a tail call. http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#newcode1501 src/x64/full-codegen-x64.cc:1501: // Array literal has ElementsKind of FAST_DOUBLE_ELEMENTS nit: missing dot at the end. http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-transitions.js File test/mjsunit/array-literal-transitions.js (right): http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-transitions.js#newcode128 test/mjsunit/array-literal-transitions.js:128: function d() { Isn't everything from here on down contained in test_large_literal() anyway? http://codereview.chromium.org/8258015/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
