LGTM
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; The +1 seems to indicate an unwarranted precision here. http://codereview.chromium.org/18708/diff/1/3#newcode1397 Line 1397: enum DeferredActionUndoType { IGNORE, RESTORE, CLEAR } undo_action = IGNORE; I think it's cleaner to create the enum and use it in two different statements. http://codereview.chromium.org/18708/diff/1/3#newcode1414 Line 1414: value = psr->value(); Here you are overwriting a value with an older one! You should just ignore the value, but use the action for the undo action. http://codereview.chromium.org/18708/diff/1/3#newcode1420 Line 1420: // counters. They have no significant previous value. This is unsafe for a loop counter inside an outer loop. http://codereview.chromium.org/18708/diff/1/3#newcode1467 Line 1467: (pushes % push_limit == 0) ? RegExpMacroAssembler::kCheckStackLimit I'd prefer just zeroing the pushlimit here. That way you avoid the expensive % operation and I think it would be clearer. http://codereview.chromium.org/18708/diff/1/3#newcode1564 Line 1564: if (!label()->is_bound()) { If you are not going to flush the trace it seems dodgy to bind the label. The label is supposed to be bound to a generic version of the node. http://codereview.chromium.org/18708 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
