LGTM
http://codereview.chromium.org/545151/diff/1/2 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/545151/diff/1/2#newcode8223 src/ia32/codegen-ia32.cc:8223: // Check for flat ascii cons string. Instead of testing for flat ASCII and Two-Byte strings independently, could you just test for a flat string (const string with second part being The Empty String), and then loop back to checking the representation and encoding with the first part? http://codereview.chromium.org/545151/diff/1/2#newcode8224 src/ia32/codegen-ia32.cc:8224: __ cmp(ebx, kStringTag | kConsStringTag | kAsciiStringTag); I.e., ignore the representation tag here, and just see if it's a cons ... http://codereview.chromium.org/545151/diff/1/2#newcode8229 src/ia32/codegen-ia32.cc:8229: __ mov(eax, FieldOperand(eax, ConsString::kFirstOffset)); ... and jump back to line 8209 here. http://codereview.chromium.org/545151/diff/1/2#newcode8241 src/ia32/codegen-ia32.cc:8241: __ cmp(ebx, kStringTag | kConsStringTag | kStringTag); Two occurrences of kStringTag and non of kTwoByteStringTag? http://codereview.chromium.org/545151/diff/1/2#newcode8261 src/ia32/codegen-ia32.cc:8261: __ mov(edx, FieldOperand(ecx, JSRegExp::kDataAsciiCodeOffset)); If we had the string's instance type here (I believe it's in ebc), we could do compute both the offset and the 0/1 for being two-byte from the type & kStringEncodingMask (ASCII being 4 and UC16 being 0). It's probably not a lot shorter than having both branches of code, though. But we could completely avoid checking the encoding above. http://codereview.chromium.org/545151/diff/1/2#newcode8275 src/ia32/codegen-ia32.cc:8275: __ xor_(edi, Operand(edi)); // Type is two byte. You can use __ Set(edi, Immediate(0)); to make this more readable. The Set macro uses xor if the argument is zero and mov if it's not. http://codereview.chromium.org/545151/diff/1/2#newcode8276 src/ia32/codegen-ia32.cc:8276: __ jmp(&invoke_regexp); Should just fall through. http://codereview.chromium.org/545151/diff/1/3 File src/regexp-macro-assembler.cc (right): http://codereview.chromium.org/545151/diff/1/3#newcode128 src/regexp-macro-assembler.cc:128: ASSERT_EQ(0, ConsString::cast(subject_ptr)->second()->length()); The thing you check against in the assembler is being equal to Heap::empty_string(), not just having length zero. Maybe make a comment that there can only be one zero-length string. http://codereview.chromium.org/545151
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
