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
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+
Radar WebKit Bug Importer
Comment 1 2016-03-20 21:47:30 PDT
Michael Saboff
Comment 2 2016-03-22 11:24:02 PDT
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
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.