Only read until regexp-stack.h, but LGTM so far.
http://codereview.chromium.org/521028/diff/1/28 File src/arm/regexp-macro-assembler-arm.cc (right): http://codereview.chromium.org/521028/diff/1/28#newcode62 src/arm/regexp-macro-assembler-arm.cc:62: * - direct_call (if 1 direct call from JavaScript code, if 0 call "1" -> "1,". http://codereview.chromium.org/521028/diff/1/28#newcode71 src/arm/regexp-macro-assembler-arm.cc:71: * - int* capture_array (int[num_saved_registers_], for output). This line should be removed, and a line for start_index added, to match the new parameter passing. http://codereview.chromium.org/521028/diff/1/28#newcode91 src/arm/regexp-macro-assembler-arm.cc:91: * int start_index, As discussed, if you pass start_index, then "at_start" isn't necessary (just create a bug entry about removing it). http://codereview.chromium.org/521028/diff/1/17 File src/assembler.h (right): http://codereview.chromium.org/521028/diff/1/17#newcode425 src/assembler.h:425: static ExternalReference address_of_memory_address(); Perhaps rename to address_of_regexp_stack_memory_address, to make it obvious which memory we are talking about. http://codereview.chromium.org/521028/diff/1/3 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/521028/diff/1/3#newcode5948 src/ia32/codegen-ia32.cc:5948: destination()->Split(less_equal); Empty line here too, for consistency. http://codereview.chromium.org/521028/diff/1/3#newcode7935 src/ia32/codegen-ia32.cc:7935: __ jmp(&runtime); That looks inefficient. Rather put the "if (FLAG_regexp_entry_native)" around the conditional part instead of generating it unconditionally and then jump past it. http://codereview.chromium.org/521028/diff/1/3#newcode7951 src/ia32/codegen-ia32.cc:7951: __ j(not_equal, &runtime, not_taken); Default for forward jumps is not_taken, so omit the hint. (Or change the assembler to not emit not_taken hints for forward jumps and taken hints for backwards jumps). This goes for all the not_taken hints below. http://codereview.chromium.org/521028/diff/1/3#newcode7956 src/ia32/codegen-ia32.cc:7956: __ CmpObjectType(ecx, FIXED_ARRAY_TYPE, ebx); Checking against undefined is not necessary. It's included in the not-fixed-array test. Make a comment that the value can never (and MUST never) be a smi. http://codereview.chromium.org/521028/diff/1/3#newcode7964 src/ia32/codegen-ia32.cc:7964: __ cmp(ebx, JSRegExp::IRREGEXP * 2); // Type is a smi. Use Smi::valueOf(JSRegExp::IRREGEXP) instead of hardcoding the conversion. No need to test for the smi tag first. If it's equal to Smi::valueof(something) then it's smi-tagged. http://codereview.chromium.org/521028/diff/1/3#newcode7972 src/ia32/codegen-ia32.cc:7972: // Calculate number of capture registers (number_of_captures + 1) * 2. Add: ASSERT_EQ(0, kSmiTag); ASSERT_EQ(1, kSmiTagSize + kSmiShiftSize); to ensure that Smi::valueOf(n) == n * 2 http://codereview.chromium.org/521028/diff/1/3#newcode7988 src/ia32/codegen-ia32.cc:7988: ASSERT_EQ(0, kSeqStringTag); Why not test for SeqAsciiString directly here, instead of testing ASCII later? (Unless you plan on supporting UC16 strings as well :) http://codereview.chromium.org/521028/diff/1/3#newcode7989 src/ia32/codegen-ia32.cc:7989: __ test(ebx, Operand(ebx)); how about just: test(ebx, Immediate(kStringRepresentationMask)) (keep the ASSERT). http://codereview.chromium.org/521028/diff/1/3#newcode8002 src/ia32/codegen-ia32.cc:8002: __ sar(eax, 1); ASSERT_EQ(1, kSmiTagSize + kSmiShiftSize); http://codereview.chromium.org/521028/diff/1/3#newcode8004 src/ia32/codegen-ia32.cc:8004: __ j(greater, &runtime); I think, but haven't checked thoroughly, that you could just set the previous_index value to zero in this case. http://codereview.chromium.org/521028/diff/1/3#newcode8020 src/ia32/codegen-ia32.cc:8020: __ j(not_equal, &runtime, not_taken); Is it necessary to check the type if we also check the map? (I.e., can any non-fixed-array every have fixed_array_map as map?) http://codereview.chromium.org/521028/diff/1/3#newcode8029 src/ia32/codegen-ia32.cc:8029: // Check the encoding of the subject string (only support ascii). Why only ASCII? UC16 should be just as simple, except for a multiplier here and there :) http://codereview.chromium.org/521028/diff/1/3#newcode8048 src/ia32/codegen-ia32.cc:8048: __ j(zero, &runtime, not_taken); Is it necessary to test both? It should be sufficient to check the size being non-zero. http://codereview.chromium.org/521028/diff/1/3#newcode8055 src/ia32/codegen-ia32.cc:8055: __ CmpObjectType(edx, CODE_TYPE, ebx); No need to test for smi mask. The value can only ever be a Code object or the_hole. Or are we assuming that the values can be maliciously tampered with? http://codereview.chromium.org/521028/diff/1/3#newcode8120 src/ia32/codegen-ia32.cc:8120: __ j(equal, &failure_or_exception, taken); Could we just do k(not_equal, &runtime) ? http://codereview.chromium.org/521028/diff/1/3#newcode8125 src/ia32/codegen-ia32.cc:8125: __ mov(Operand(eax), Factory::null_value()); Where are we handling the pending exception? The runtime call would handle it during JS-reentry, but I think we need to throw manually here. http://codereview.chromium.org/521028/diff/1/3#newcode8137 src/ia32/codegen-ia32.cc:8137: // Load last_match_info which is still known to be a fast case JSArray. Well, it's still assumed to be one. I'm sure someone malicious with the debugger could break that assumption by breaking inside the regexp and fiddling with the array (if it's visible somehow). http://codereview.chromium.org/521028/diff/1/3#newcode8177 src/ia32/codegen-ia32.cc:8177: __ add(edi, Operand(esp, 2 * kPointerSize)); // Adding smi to smi. Is there really no register free to hold this value? What if we were counting down instead of up, so we didin't need both the counter and the limit? We are reading it repeatedly from memory inside a loop (I know it'll be in the level-1 cache, but still). http://codereview.chromium.org/521028 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
