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?
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. :-)
Created attachment 131298 [details] Patch
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.
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.
> 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.
(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).
Created attachment 131540 [details] Patch
Comment on attachment 131540 [details] Patch Thanks for the fix - looks great!
Comment on attachment 131540 [details] Patch Clearing flags on attachment: 131540 Committed r110540: <http://trac.webkit.org/changeset/110540>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > (From update of attachment 131540 [details]) > Thanks for the fix - looks great! Thank you for review