Bug 155315 – [ES6] Allow RegExp constructor to take pattern from an existing RegExp with new flags

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

[Michael Saboff]

Currently the following is a syntax error:
  var re = new RegExp(/a/i, "m")

ES6 allows this with the resulting RegExp taking the pattern from the passed RegExp and the provided flags.

[Radar WebKit Bug Importer]

<rdar://problem/25091247>

[Michael Saboff]

Created attachment 273597 [details]
Patch

[Saam Barati]

Comment on attachment 273597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273597&action=review

r=me with comments

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:268
> +        if (exec->hadException())

should be vm.exception()

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:271
> +            return vm.throwException(exec, createSyntaxError(exec, ASCIILiteral("Invalid flags supplied to RegExp constructor.")));

Do we have tests for this exception?

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:284
> +                regExp = RegExp::create(vm, regExp->pattern(), flags);

Maybe this should have an OOM exception check?

[Build Bot]

Comment on attachment 273597 [details]
Patch

Attachment 273597 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/956056

New failing tests:
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T8.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T2.html

[Build Bot]

Created attachment 273606 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 273597 [details]
Patch

Attachment 273597 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/956064

New failing tests:
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T8.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T2.html

[Build Bot]

Created attachment 273607 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 273597 [details]
Patch

Attachment 273597 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/956072

New failing tests:
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T8.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T2.html

[Build Bot]

Created attachment 273609 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 273597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273597&action=review
> 
> r=me with comments
> 
> > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:268
> > +        if (exec->hadException())
> 
> should be vm.exception()

I made this change throughout the function.

> > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:271
> > +            return vm.throwException(exec, createSyntaxError(exec, ASCIILiteral("Invalid flags supplied to RegExp constructor.")));
> 
> Do we have tests for this exception?

Added.

> > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:284
> > +                regExp = RegExp::create(vm, regExp->pattern(), flags);
> 
> Maybe this should have an OOM exception check?

Added.

I'll fix the sputnik tests and land.

[Build Bot]

Comment on attachment 273597 [details]
Patch

Attachment 273597 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/956089

New failing tests:
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T8.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.3/S15.10.3.1_A2_T1.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A2_T2.html

[Build Bot]

Created attachment 273613 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Michael Saboff]

Created attachment 273616 [details]
Patch for Landing - Made suggested changes, removed obsolete sputnik tests

[Michael Saboff]

Committed r197962: <http://trac.webkit.org/changeset/197962>

[Darin Adler]

Comment on attachment 273616 [details]
Patch for Landing - Made suggested changes, removed obsolete sputnik tests

View in context: https://bugs.webkit.org/attachment.cgi?id=273616&action=review

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:265
> +    RegExpFlags flags = NoFlags;
> +    bool haveFlags = false;

I think it would be elegant to use Optional<RegExpFlags> rather than a separate boolean. Might return to do that refactoring later.

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:269
> +            return 0;

nullptr