WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
155711
[ES6] Implement RegExp.prototype[@@match]
https://bugs.webkit.org/show_bug.cgi?id=155711
Summary
[ES6] Implement RegExp.prototype[@@match]
Michael Saboff
Reported
2016-03-20 21:47:06 PDT
Implement RegExp.prototype[@@match] and have String.prototype.match per sections 21.1.3.11 and 21.2.5.6 of the ECMAScript 2016 standard.
Attachments
Patch
(44.53 KB, patch)
2016-03-22 11:24 PDT
,
Michael Saboff
no flags
Details
Formatted Diff
Diff
Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec.
(44.26 KB, patch)
2016-03-22 13:22 PDT
,
Michael Saboff
fpizlo
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-03-20 21:47:30 PDT
<
rdar://problem/25263672
>
Michael Saboff
Comment 2
2016-03-22 11:24:02 PDT
Created
attachment 274668
[details]
Patch
Michael Saboff
Comment 3
2016-03-22 13:22:16 PDT
Created
attachment 274678
[details]
Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec.
Michael Saboff
Comment 4
2016-03-22 14:42:06 PDT
Committed
r198554
: <
http://trac.webkit.org/changeset/198554
>
Saam Barati
Comment 5
2016-03-22 14:45:25 PDT
Comment on
attachment 274678
[details]
Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec. View in context:
https://bugs.webkit.org/attachment.cgi?id=274678&action=review
r=me with the noted suggestions. I think it's also worth adding a test or two that uses Proxy to verify that operations are performed only the number of needed times, in the right order, etc.
> Source/JavaScriptCore/builtins/RegExpPrototype.js:59 > + if (regexp.exec !== @RegExp.prototype.@exec && typeof(regexp.exec) === "function") {
Style: I think our "typeof" style is usually this: typeof regex.exec === "function" Also, I'm not sure this is correct. You're calling "get" twice here. That is an observable operation. It looks like RegExpExec only does this get once in the spec.
> Source/JavaScriptCore/builtins/StringPrototype.js:40 > + var matcher = regexp[@symbolMatch];
This isn't correct w.r.t to the spec. It looks like the spec calls GetMethod here. GetMethod will throw if the result isn't undefined or null. So if this returned something like 42, then we should throw. If it returned undefined or null, we shouldn't throw.
Csaba Osztrogonác
Comment 6
2016-03-23 00:00:47 PDT
(In reply to
comment #4
)
> Committed
r198554
: <
http://trac.webkit.org/changeset/198554
>
It caused a serious regression on ARMv7 Thumb2 platforms:
bug155784
Yusuke Suzuki
Comment 7
2016-03-23 00:20:02 PDT
Comment on
attachment 274678
[details]
Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec. View in context:
https://bugs.webkit.org/attachment.cgi?id=274678&action=review
>> Source/JavaScriptCore/builtins/StringPrototype.js:40 >> + var matcher = regexp[@symbolMatch]; > > This isn't correct w.r.t to the spec. > It looks like the spec calls GetMethod here. GetMethod will throw if the result isn't undefined or null. > So if this returned something like 42, then we should throw. If it returned undefined or null, > we shouldn't throw.
Ah, oops! I missed that when implementing String.prototype.search, nice! I think `if (searcher !== @undefined)` to `if (searcher != @undefined)` will solve the issue. If the returned value is non undefined/null and non-function value (like 42), `searcher.@call(regexp, this);` will throw an exception, "Exception: TypeError: 42 is not a function". I'll upload the patch fixing the issues in String.prototype.match and String.prototype.search :D
Michael Saboff
Comment 8
2016-03-23 14:13:08 PDT
(In reply to
comment #5
)
> Comment on
attachment 274678
[details]
> Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to > only check that it is an object, per spec. > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=274678&action=review
> > r=me with the noted suggestions. > I think it's also worth adding a test or two that uses Proxy to verify that > operations are performed only the number of needed times, in the right > order, etc.
>
> > Source/JavaScriptCore/builtins/RegExpPrototype.js:59 > > + if (regexp.exec !== @RegExp.prototype.@exec && typeof(regexp.exec) === "function") { > > Style: I think our "typeof" style is usually this: typeof regex.exec === > "function" > Also, I'm not sure this is correct. You're calling "get" twice here. That is > an observable > operation. It looks like RegExpExec only does this get once in the spec.
Going to handle this issue as well as adding Proxy based tests in a new patch. Tracked with <
https://bugs.webkit.org/show_bug.cgi?id=155807
>
> > Source/JavaScriptCore/builtins/StringPrototype.js:40 > > + var matcher = regexp[@symbolMatch]; > > This isn't correct w.r.t to the spec. > It looks like the spec calls GetMethod here. GetMethod will throw if the > result isn't undefined or null. > So if this returned something like 42, then we should throw. If it returned > undefined or null, > we shouldn't throw.
This was handled in <
https://bugs.webkit.org/show_bug.cgi?id=155785
> and landed in change set <
http://trac.webkit.org/changeset/198581
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug