Addressed review comments
http://codereview.chromium.org/18708/diff/1/3 File src/jsregexp.cc (right): http://codereview.chromium.org/18708/diff/1/3#newcode1385 Line 1385: const int push_limit = (assembler->stack_limit_slack() + 1) / 2; Ack, it lost its comment. The +1 is to avoid doing modulo with zero if the slack is 1 (one). http://codereview.chromium.org/18708/diff/1/3#newcode1397 Line 1397: enum DeferredActionUndoType { IGNORE, RESTORE, CLEAR } undo_action = IGNORE; On 2009/01/23 13:09:21, Erik Corry wrote: > I think it's cleaner to create the enum and use it in two different statements. Done. http://codereview.chromium.org/18708/diff/1/3#newcode1414 Line 1414: value = psr->value(); On 2009/01/23 13:09:21, Erik Corry wrote: > Here you are overwriting a value with an older one! You should just ignore the > value, but use the action for the undo action. Done. http://codereview.chromium.org/18708/diff/1/3#newcode1420 Line 1420: // counters. They have no significant previous value. Correct. We will have to propagate the information on whether we are inside a loop to this point. Until then, we'll have to restore the value to be safe. http://codereview.chromium.org/18708/diff/1/3#newcode1440 Line 1440: undo_action = CLEAR; This is also a problem if the STORE_POSITION is not a capture, but for checking if a loop matches emptily. If the loop is inside another loop, we need to restore. http://codereview.chromium.org/18708/diff/1/3#newcode1467 Line 1467: (pushes % push_limit == 0) ? RegExpMacroAssembler::kCheckStackLimit On 2009/01/23 13:09:21, Erik Corry wrote: > I'd prefer just zeroing the pushlimit here. That way you avoid the expensive % > operation and I think it would be clearer. Done. http://codereview.chromium.org/18708 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
