Irregexp:...

Irregexp:
* Fix UC16 character classes on ASCII subjects.
* Fix sign problem in Irregexp interpreter.
* Make passes over text nodes more readable.

Committed: http://code.google.com/p/v8/source/detail?r=1304

[Lasse Reichstein, 2009-02-18 15:20:44]

LGTM.

http://codereview.chromium.org/21450/diff/1/5
File src/jsregexp.cc (right):

http://codereview.chromium.org/21450/diff/1/5#newcode2273
Line 2273: pos->determines_perfectly = false;
Should this be set to false? If the remaining cases of a choice node are
actually determined perfectly, we should match them.
I.e., treat a cannot-match result as if it wasn't there at all.

http://codereview.chromium.org/21450/diff/1/5#newcode2329
Line 2329: // succeed.
This comment should be changed to just "not yet handled".
It would be fairly easy to normalize the character class representation to be
non-negated and treat it like a positive class.

http://codereview.chromium.org/21450/diff/1/5#newcode2677
Line 2677: return quick_check->positions(offset)->determines_perfectly;
Just checking: If we find that a position determines the branch perfectly, will
the position always be tested? Or is it possible that, e.g., both the first and
second character perfectly determines the branch, and only the first is actually
checked?

http://codereview.chromium.org/21450/diff/1/5#newcode2722
Line 2722: int* checked_up_to) {
This function is one big glob of special cases.
If the different passes has something in common, please refactor it to, e.g., a
template function or pass objects with virtual methods or something else that
avoids putting all the cases in one monolithic function.

[Erik Corry, 2009-02-18 16:04:24]

http://codereview.chromium.org/21450/diff/1/5
File src/jsregexp.cc (right):

http://codereview.chromium.org/21450/diff/1/5#newcode2273
Line 2273: pos->determines_perfectly = false;
On 2009/02/18 15:20:44, Lasse Reichstein wrote:
> Should this be set to false? If the remaining cases of a choice node are
> actually determined perfectly, we should match them.
> I.e., treat a cannot-match result as if it wasn't there at all.

Determines perfectly indicates that this particular character position has been
checked completely by the quick check.  The implication is that if we get past
the quick check then we don't need to look at this character position again.  So
'false' would be wrong here.

http://codereview.chromium.org/21450/diff/1/5#newcode2329
Line 2329: // succeed.
On 2009/02/18 15:20:44, Lasse Reichstein wrote:
> This comment should be changed to just "not yet handled".
> It would be fairly easy to normalize the character class representation to be
> non-negated and treat it like a positive class.

I don't think that char classes like [^\u0000-ac-\uffff] are common enough to
warrant spending time on, ever.

http://codereview.chromium.org/21450/diff/1/5#newcode2677
Line 2677: return quick_check->positions(offset)->determines_perfectly;
On 2009/02/18 15:20:44, Lasse Reichstein wrote:
> Just checking: If we find that a position determines the branch perfectly,
will
> the position always be tested? Or is it possible that, e.g., both the first
and
> second character perfectly determines the branch, and only the first is
actually
> checked?

See above.

http://codereview.chromium.org/21450/diff/1/5#newcode2722
Line 2722: int* checked_up_to) {
On 2009/02/18 15:20:44, Lasse Reichstein wrote:
> This function is one big glob of special cases.
> If the different passes has something in common, please refactor it to, e.g.,
a
> template function or pass objects with virtual methods or something else that
> avoids putting all the cases in one monolithic function.

I've refactored a little more.  It is now only 68 lines with 30 lines of
comments to explain what it is doing.  I feel that making it much smaller will
either cause code duplication or unnecessary abstraction boilerplace elsewhere.