[webkit-reviews] review denied: [Bug 6257] Throw errors on invalid expressions/support non-PCRE regexp (KJS merge) : [Attachment 5308] patch, with additional \u support

[bugzilla-request-daemon at opendarwin.org]

Geoffrey Garen <ggaren at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 6257: Throw errors on invalid expressions/support non-PCRE regexp (KJS
merge)
http://bugzilla.opendarwin.org/show_bug.cgi?id=6257

Attachment 5308: patch, with additional \u support
http://bugzilla.opendarwin.org/attachment.cgi?id=5308&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.