LGTM
http://codereview.chromium.org/17416/diff/262/404 File src/jsregexp.cc (right): http://codereview.chromium.org/17416/diff/262/404#newcode1353 Line 1353: int push_limit = (assembler->stack_limit() + 1) / 2; Why plus 1? http://codereview.chromium.org/17416/diff/262/404#newcode1471 Line 1471: assembler->PushCurrentPosition(RegExpMacroAssembler::kNoStackLimitCheck); We never emit this with stack limit check so the extra boolean is just noise. http://codereview.chromium.org/17416/diff/262/404#newcode1479 Line 1479: assembler->PushBacktrack(&undo, RegExpMacroAssembler::kCheckStackLimit); We should always emit this with stack limit check so the extra boolean is just noise. http://codereview.chromium.org/17416/diff/262/404#newcode2813 Line 2813: RegExpMacroAssembler::kCheckStackLimit); We don't need to check here. Greedy loops cannot contain other stack operations (or they would not work) and are not nested. http://codereview.chromium.org/17416/diff/262/405 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/262/405#newcode665 Line 665: __ mov(ecx, -1); // Word offset of register 0 relative to ebp. I can't make out what this comment above means. http://codereview.chromium.org/17416/diff/262/405#newcode671 Line 671: __ dec(ecx); Please avoid dec due to partial flags update performance hit. http://codereview.chromium.org/17416/diff/262/405#newcode680 Line 680: i < num_registers_; i += kRegistersPerPage) { for(...;...;...) should be either all on one line or on 3 lines. http://codereview.chromium.org/17416/diff/262/405#newcode681 Line 681: __ mov(register_location(i), eax); // One write every page. Please add a test that executes this line if there isn't one already. http://codereview.chromium.org/17416/diff/262/405#newcode876 Line 876: if (check_stack_limit) CheckStackLimit(); Checking twice just to be sure? :-) http://codereview.chromium.org/17416/diff/262/412 File src/regexp-macro-assembler.h (right): http://codereview.chromium.org/17416/diff/262/412#newcode57 Line 57: // often is allowed (and encouraged) Missing full stop, and I see no need to encourage inefficiency. Also the name is not descriptive. Something including the word 'slack' seems more appropriate. http://codereview.chromium.org/17416/diff/262/414 File src/regexp-stack.h (right): http://codereview.chromium.org/17416/diff/262/414#newcode64 Line 64: return &(thread_local_.limit_); This looks like it would all fit on one line. http://codereview.chromium.org/17416 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
