LGTM with comments.
-- Vitaly http://codereview.chromium.org/3158020/diff/1/2 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/3158020/diff/1/2#newcode5301 src/arm/codegen-arm.cc:5301: __ ldm(ib, r0, r3.bit() | r4.bit() | r5.bit() | r6.bit() | r7.bit()); See ia32 comments on requirements and assertions below. http://codereview.chromium.org/3158020/diff/1/2#newcode5308 src/arm/codegen-arm.cc:5308: __ str(ip, FieldMemOperand(r4, HeapObject::kMapOffset)); Move this one line up? http://codereview.chromium.org/3158020/diff/1/9 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/3158020/diff/1/9#newcode5555 src/ia32/codegen-ia32.cc:5555: __ push(save_register); push -> pop. http://codereview.chromium.org/3158020/diff/1/9#newcode7402 src/ia32/codegen-ia32.cc:7402: __ cmp(edx, FieldOperand(eax, HeapObject::kMapOffset)); Please document when we expect it to have a different map. http://codereview.chromium.org/3158020/diff/1/9#newcode7421 src/ia32/codegen-ia32.cc:7421: // Copy properties array of input (should be empty FixedArray). Consider generating the initialization code in a loop like this: for (offset = kPointerSize; offset < JSRegExpResult::kSize; offset += kPointerSize) You can add a special case for the elements offset and change the map there. Changing the map requires that the elements array is not empty (the empty array is a singleton). Is this requirement satisfied here? Also consider adding an assert that the property array is empty. http://codereview.chromium.org/3158020/diff/1/13 File src/runtime.cc (right): http://codereview.chromium.org/3158020/diff/1/13#newcode1378 src/runtime.cc:1378: if (result->map() != regexp_result_map) { nit: Can be a one liner. http://codereview.chromium.org/3158020/diff/1/13#newcode1379 src/runtime.cc:1379: return result; Please document when this is supposed to happen. http://codereview.chromium.org/3158020/diff/1/13#newcode1382 src/runtime.cc:1382: ASSERT(result->HasFastElements()); Mention that we don't need runtime checks here because this structure is guaranteed by the regexp_result_map. http://codereview.chromium.org/3158020/diff/1/13#newcode1387 src/runtime.cc:1387: NEW_SPACE, nit: Align parameters. http://codereview.chromium.org/3158020/diff/1/13#newcode1397 src/runtime.cc:1397: new_array->set_properties(result->properties()); // Is empty FixedArray. "Is empty" -> "Empty". http://codereview.chromium.org/3158020/diff/1/13#newcode1400 src/runtime.cc:1400: FixedArray* elements = FixedArray::cast(result->elements()); Assert !elements->IsEmpty(). See ia32 comments above. http://codereview.chromium.org/3158020/diff/1/13#newcode1403 src/runtime.cc:1403: new_array->set_elements(result->elements()); // Is empty FixedArray. Remove this line. http://codereview.chromium.org/3158020/diff/1/15 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/3158020/diff/1/15#newcode4834 src/x64/codegen-x64.cc:4834: __ push(save_register); push -> pop. http://codereview.chromium.org/3158020/diff/1/15#newcode6637 src/x64/codegen-x64.cc:6637: // Load JSRegExp map into edx. Check that argument object has this map. edx -> rdx. http://codereview.chromium.org/3158020/diff/1/15#newcode6662 src/x64/codegen-x64.cc:6662: // Copy properties array of input (should be empty FixedArray). See ia32 comments above. http://codereview.chromium.org/3158020/diff/1/18 File test/mjsunit/regexp.js (right): http://codereview.chromium.org/3158020/diff/1/18#newcode488 test/mjsunit/regexp.js:488: // Test that caching of result doesn't share result objects. Put this code in a loop to increase the likelihood of exercising GC-related paths. http://codereview.chromium.org/3158020/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
