Bug 155315 - [ES6] Allow RegExp constructor to take pattern from an existing RegExp with new flags
Summary: [ES6] Allow RegExp constructor to take pattern from an existing RegExp with n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-10 12:12 PST by Michael Saboff
Modified: 2016-03-13 14:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.45 KB, patch)
2016-03-10 12:26 PST, Michael Saboff
sbarati: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (788.05 KB, application/zip)
2016-03-10 13:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (988.75 KB, application/zip)
2016-03-10 13:13 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (643.64 KB, application/zip)
2016-03-10 13:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (855.94 KB, application/zip)
2016-03-10 13:35 PST, Build Bot
no flags Details
Patch for Landing - Made suggested changes, removed obsolete sputnik tests (26.49 KB, patch)
2016-03-10 13:56 PST, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2016-03-10 12:12:48 PST
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.
Comment 1 Radar WebKit Bug Importer 2016-03-10 12:13:34 PST
<rdar://problem/25091247>
Comment 2 Michael Saboff 2016-03-10 12:26:24 PST
Created attachment 273597 [details]
Patch
Comment 3 Saam Barati 2016-03-10 12:33:03 PST
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?
Comment 4 Build Bot 2016-03-10 13:09:57 PST
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
Comment 5 Build Bot 2016-03-10 13:10:00 PST
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
Comment 6 Build Bot 2016-03-10 13:13:27 PST
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
Comment 7 Build Bot 2016-03-10 13:13:29 PST
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
Comment 8 Build Bot 2016-03-10 13:18:15 PST
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
Comment 9 Build Bot 2016-03-10 13:18:17 PST
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
Comment 10 Michael Saboff 2016-03-10 13:21:19 PST
(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.
Comment 11 Build Bot 2016-03-10 13:35:22 PST
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
Comment 12 Build Bot 2016-03-10 13:35:24 PST
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
Comment 13 Michael Saboff 2016-03-10 13:56:00 PST
Created attachment 273616 [details]
Patch for Landing - Made suggested changes, removed obsolete sputnik tests
Comment 14 Michael Saboff 2016-03-10 15:38:02 PST
Committed r197962: <http://trac.webkit.org/changeset/197962>
Comment 15 Darin Adler 2016-03-13 14:58:21 PDT
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