289628 - Failed emulation of Perl RegExp quantifiers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

Current SM 1.7.7/trunk and FF 1.0.3/trunk builds fail 

sections 13, 14, 17, 20-27 of 
http://lxr.mozilla.org/mozilla/source/js/tests/ecma_3/RegExp/regress-188206.js

sections 21, 23 of 
http://lxr.mozilla.org/mozilla/source/js/tests/ecma_3/RegExp/regress-223273.js

sections 6-10, 12-13, 15-17 of 
http://lxr.mozilla.org/mozilla/source/js/tests/ecma_3/RegExp/regress-228087.js

Note MSIE passes each of these sections.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: imitate perl

Please ignore the js_ReportCompileErrorNumberUC change. I'll make sure to check
those changes in seperately. This makes us pass all of the tests, though I'm
slightly confused since I could have sworn I read a Rhino patch that only
allowed /{<non-digit>/ to be treated as flat. PERL seems to agree with this
patch too, however, so I'm going to leave it as is.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: imitate perl more cleanly

The goto was unnecessary, it was added when I still thought that /{3/ was an
invalid regular expression.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 189375 [details] [diff] [review]
imitate perl more cleanly

r+a=me, nice minimal fix.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

*sigh*, my fix was too minimal. Running the JS testsuite showed me what I'd
missed before. New patch coming up with real Perl compat.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 189375 [details] [diff] [review]
imitate perl more cleanly

Marking r- and obsolete to be clear.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: imitate perl more

This isn't quite the patch that we discussed earlier. It became a lot easier to
simply add a parameter to ParseMinMaxQuantifier. Thist passes the regexp test
suite (in tests/ecma_3/RegExp) with one exception, which I'll attach as a
seperate patch to the test (it was expecting /\d{1,s}/ to be considered
invalid, but Perl treats the invalid quantifier as flat).

Note that neither IE, Safari, nor Opera allow /\d{1,s}/, so I'm not sure
whether to follow them or Perl. This patch follows Perl (though it could be
converted to follow IE/Safari/Opera).

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 189483 [details] [diff] [review]
imitate perl more

So mrbkap and I talked and we agreed to follow browsers more than Perl, in odd
cases such as this one.

But I forget already what Firefox 1.0.x does -- we may be torn between
compatibility with ourselves vs. other browsers.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 189483 [details] [diff] [review]
imitate perl more

Actually, it seems that my testcase was flawed. This does make us compatible
with IE and Perl.

I'm less worried about breaking compat. with Firefox 1.0.x, since we're
becoming more lenient and therefore we're not going to break any existing
scripts. Authors will just need to be more careful about typing out their
regexps.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to trunk

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 193583 [details] [diff] [review]
updated to trunk

>+static intN ParseMinMaxQuantifier(CompilerState *state, JSBool ignorevalues);

ignoreValues

>+            if (ParseMinMaxQuantifier(state, JS_TRUE) < 0) {
>+                /* 
>+                 * This didn't even scan correctly as a quantifier, treat it as
>+                 * flat.

"so we should" before "treat".

>+        if (!ignorevalues && min == OVERFLOW_VALUE) {
>+            return JSMSG_MIN_TOO_BIG;
>+        }

Wah.

>+        if (c == ',') {
>+            c = *++state->cp;
>+            if (JS7_ISDEC(c)) {
>+                ++state->cp;
>+                max = GetDecimalValue(c, 0xFFFF, NULL, state);
>+                c = *state->cp;
>+                if (!ignorevalues && max == OVERFLOW_VALUE) {
>+                    return JSMSG_MAX_TOO_BIG;
>+                }

Wahh.

>+                if (!ignorevalues && min > max) {
>+                    return JSMSG_OUT_OF_ORDER;
>+                }

Wahhh.

r=me with nits picked.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH and trunk.