Bug 80822 – Tests for RegExp multiline failed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hojong Han]

Testcase js1_2/regexp/RegExp_multiline.js failed 
Failure messages were:
(multiline == true) '123\n456'.match(/^4../) = null FAILED! expected: 456 
(multiline == true) 'a11\na22\na23\na24'.match(/^a../g) = a11 FAILED! expected: a11,a22,a23,a24 
(multiline == true) '123\n456'.match(/.3$/) = null FAILED! expected: 23 
(multiline == true) 'a11\na22\na23\na24'.match(/a..$/g) = a24 FAILED! expected: a11,a22,a23,a24 
(multiline == true) 'a11\na22\na23\na24'.match(new RegExp('a..$','g')) = a24 FAILED! expected: a11,a22,a23,a24 

Testcase js1_2/regexp/RegExp_multiline_as_array.js failed 
Failure messages were:
(['$*'] == true) '123\n456'.match(/^4../) = null FAILED! expected: 456 
(['$*'] == true) 'a11\na22\na23\na24'.match(/^a../g) = a11 FAILED! expected: a11,a22,a23,a24 
(['$*'] == true) '123\n456'.match(/.3$/) = null FAILED! expected: 23 
(['$*'] == true) 'a11\na22\na23\na24'.match(/a..$/g) = a24 FAILED! expected: a11,a22,a23,a24 
(['$*'] == true) 'a11\na22\na23\na24'.match(new RegExp('a..$','g')) = a24 FAILED! expected: a11,a22,a23,a24 

Testcase js1_2/regexp/beginLine.js failed 
Failure messages were:
123xyz'.match(new RegExp('^\d+')) = null FAILED! expected: 123 

Testcase js1_2/regexp/endLine.js failed 
Failure messages were:
xyz'.match(new RegExp('\d+$')) = null FAILED! expected: 890

These failed testcases are caused by creating RegExp instances without multiline flag, 'm'.
In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
Fix testcases or JSC, otherwise just let things untouched. Which one is right?

[Gavin Barraclough]

RegExp.multiline is a read-only property (see ES5.1 15.10.7.4), so these are error in the test case.  For these tests, probably best to change them to postfix the 'm' flag, if that will work.

This reminds me, I should check that strict-mode assignment to multiline etc throws. :-)

[Hojong Han]

Created attachment 131298 [details]
Patch

[WebKit Review Bot]

Attachment 131298 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:87:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:91:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:99:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:103:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:107:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/endLine.js:64:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:87:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:91:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:99:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:103:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:107:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/beginLine.js:64:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 12 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 131298 [details]
Patch

Patch looks great, but we do require just spaces, not tab characters.  Please fix the style issue, but the change looks fine.

[Geoffrey Garen]

> In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
> Fix testcases or JSC, otherwise just let things untouched. Which one is right?

I'd suggest testing in other modern browsers and seeing what they do.

[Gavin Barraclough]

(In reply to comment #5)
> > In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
> > Fix testcases or JSC, otherwise just let things untouched. Which one is right?
> 
> I'd suggest testing in other modern browsers and seeing what they do.

Just to clarify, the comment re not being in the spec is in error; behavior here is well defined (and our implementation is correct).

[Hojong Han]

Created attachment 131540 [details]
Patch

[Gavin Barraclough]

Comment on attachment 131540 [details]
Patch

Thanks for the fix - looks great!

[WebKit Review Bot]

Comment on attachment 131540 [details]
Patch

Clearing flags on attachment: 131540

Committed r110540: <http://trac.webkit.org/changeset/110540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hojong Han]

(In reply to comment #8)
> (From update of attachment 131540 [details])
> Thanks for the fix - looks great!
Thank you for review