Irregexp Issue 187: Captures in look-aheads are correctly cleared when backtracking.

Clears captures of look-aheads on backtrack.
Reduces number of pushes when flushing a trace. Some are converted to clears
in the undo-code instead, and some just ignored if they have no value worth restoring.

[Lasse Reichstein, 2009-01-23 12:07:09]

Largish code review.

[Erik Corry, 2009-01-23 13:09:21]

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.

[Lasse Reichstein, 2009-01-23 13:34:34]

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.