Comments addressed and landed.
http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc#newcode4850 src/arm/code-stubs-arm.cc:4850: // Argument 6: Clear the number of capture registers for non-global capture. On 2012/05/22 08:32:46, Erik Corry wrote:
I don't understand this comment. Is argument 6 a boolean argument
that
indicates whether we should clear the number of capture registers when
the
capture is non-global? Does it have any meaning if the regexp is
non-global? Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc#newcode4901 src/arm/code-stubs-arm.cc:4901: // We do not expect multiple results. On 2012/05/22 08:32:46, Erik Corry wrote:
This comment does not clarify much. Why do we not expect multiple
results here. Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc File src/arm/regexp-macro-assembler-arm.cc (right): http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode45 src/arm/regexp-macro-assembler-arm.cc:45: * This assembler uses the following register assignment convention On 2012/05/22 08:32:46, Erik Corry wrote:
This needs updating to include r4
Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode78 src/arm/regexp-macro-assembler-arm.cc:78: * - fp[-12] start index (character index of start). On 2012/05/22 08:32:46, Erik Corry wrote:
Inconsistent capitalization, not started by you.
Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode80 src/arm/regexp-macro-assembler-arm.cc:80: * - fp[-20] success counter (only useful for global regexp to count matches) On 2012/05/22 08:32:46, Erik Corry wrote:
Does this lint?
Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode745 src/arm/regexp-macro-assembler-arm.cc:745: __ mov(current_character(), Operand('\n'), LeaveCC, eq); On 2012/05/22 08:32:46, Erik Corry wrote:
Having conditional instructions here after non-conditional
instructions that do
not affect the flags - that is too tricky. Please refactor to use
conventional
if-then-else branching.
Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode755 src/arm/regexp-macro-assembler-arm.cc:755: LoadCurrentCharacterUnchecked(-1, 1); On 2012/05/22 08:32:46, Erik Corry wrote:
In the global case the generated code isn't making much sense. It's
doing:
Load CurrentChar jmp loaded Load CurrentChar bind loaded
Done. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode894 src/arm/regexp-macro-assembler-arm.cc:894: // String might have moved: Reload end of string from frame. On 2012/05/22 08:32:46, Erik Corry wrote:
If this happens will r4 be updated with the correct position?
r4 is a relative index from the end of string, therefore should not be affected from moving the string. http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assembler-arm.cc#newcode1378 src/arm/regexp-macro-assembler-arm.cc:1378: offset = ip; On 2012/05/22 08:32:46, Erik Corry wrote:
This is setting a trap for a future programmer. There are lots of
instructions
that implicitly overwrite ip, so we should not use ip unless we have
to, and if
we do it should be over 1-2 lines so you don't overlook that it is
taken. Using r4 now since it's not being used at this point. http://codereview.chromium.org/10386090/diff/3002/src/jsregexp.cc File src/jsregexp.cc (right): http://codereview.chromium.org/10386090/diff/3002/src/jsregexp.cc#newcode5808 src/jsregexp.cc:5808: RegExpCompiler compiler(data->capture_count, ignore_case, is_ascii); On 2012/05/22 08:32:46, Erik Corry wrote:
Optimization idea (probably for the next patch): At this point you can
use
data->tree->min_match() to see whether the regexp can do a zero length
match.
You can pass that as a flag to the compiler and it can tell the macro
assembler
to avoid testing for zero length matches in the repeat code on global
regexps. This is exactly what I've been looking for! I tried my luck with EatsAtLeast, but that wouldn't work for non-capturing matches (e.g. lookahead). Thanks! http://codereview.chromium.org/10386090/diff/3002/test/mjsunit/regexp-global.js File test/mjsunit/regexp-global.js (right): http://codereview.chromium.org/10386090/diff/3002/test/mjsunit/regexp-global.js#newcode50 test/mjsunit/regexp-global.js:50: assertEquals("2It 3was 1a 8pleasure 2to 4burn.", str); On 2012/05/22 08:32:46, Erik Corry wrote:
Nice test :-)
:) http://codereview.chromium.org/10386090/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
