Addressed review comments.
http://codereview.chromium.org/17416/diff/245/250 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/245/250#newcode695 Line 695: // stack limit being hit and an exception have already been raised. On 2009/01/10 07:31:39, iposva wrote: > "an exception has already" Done. 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; Rounding up in case the value is one (which it happens to be for macro-assembler-irregexp). Comment added. http://codereview.chromium.org/17416/diff/262/404#newcode1471 Line 1471: assembler->PushCurrentPosition(RegExpMacroAssembler::kNoStackLimitCheck); What's wrong with needless generality now?!? Ok, removed. http://codereview.chromium.org/17416/diff/262/404#newcode1479 Line 1479: assembler->PushBacktrack(&undo, RegExpMacroAssembler::kCheckStackLimit); Removed too http://codereview.chromium.org/17416/diff/262/404#newcode2813 Line 2813: RegExpMacroAssembler::kCheckStackLimit); PushCurrentPosition never checks now. We shouldn't need more than one of those between other types of pushes anyway. http://codereview.chromium.org/17416/diff/262/405 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/262/405#newcode680 Line 680: i < num_registers_; i += kRegistersPerPage) { On 2009/01/12 11:50:13, Erik Corry wrote: > for(...;...;...) should be either all on one line or on 3 lines. Done. http://codereview.chromium.org/17416/diff/262/405#newcode681 Line 681: __ mov(register_location(i), eax); // One write every page. On 2009/01/12 11:50:13, Erik Corry wrote: > Please add a test that executes this line if there isn't one already. Done. http://codereview.chromium.org/17416/diff/262/405#newcode876 Line 876: if (check_stack_limit) CheckStackLimit(); Better safe and safe than sorry and sorry. Fixed. 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) On 2009/01/12 11:50:13, Erik Corry wrote: > 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. Done. 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_); On 2009/01/12 11:50:13, Erik Corry wrote: > This looks like it would all fit on one line. Done. http://codereview.chromium.org/17416 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
