[Webkit-unassigned] [Bug 6257] Throw errors on invalid expressions/support non-PCRE regexp (KJS merge)

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=6257


ggaren at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #5308|review?(ggaren at apple.com)   |review-
               Flag|                            |




------- Additional Comments From ggaren at apple.com  2006-01-09 14:36 -------
(From update of attachment 5308)
Sorry I took so long to review this.

1. ecma_3/RegExp/regress-119909.js. We can't check in code that causes test
regressions, so we need to figure out why this test still fails with your
latest patch. I suspect it's because (a) throwing an error prevents the rest of
the test from running or (b) testkjs interprets all thrown errors as failures.
Either way, the simplest solution is probably to surround the relevant part of
the test in a try/catch block.

2. Performance. I'm worried about the performance of scanning the expression
string an extra time. We've had performance problems in this area before. At a
minimum, I would suggest changing the code so that we only scan for \u if pcre
returns an error with the errorCode corresponding to "PCRE does not support \u
in RegExps." However, I think a better approach would be to have the lexer take
care of converting the unicode expression from the get-go.

3. To match flags() / _flags, _valid should be _isValid.

4. When pcre fails, doesn't it set _regex to NULL? If so, you can eliminate
_isValid, and have isValid() just return _regex.

r- for 1 & 2, but I'd love to see a patch that covers 3 & 4, too.

I hope this review doesn't sound too negative. You've been doing great work,
and we've had a lot of requests to fix this bug in particular, so it's great
that you're working on it.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.