[webkit-reviews] review granted: [Bug 204490] [JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace] : [Attachment 384180] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 204490: [JSC] GetSubstitution is performed incorrectly via
RegExp.prototype[@@replace]
https://bugs.webkit.org/show_bug.cgi?id=204490

Attachment 384180: Patch

https://bugs.webkit.org/attachment.cgi?id=384180&action=review




--- Comment #7 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 384180
  --> https://bugs.webkit.org/attachment.cgi?id=384180
Patch

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

Nice tests.  r=me with fixes.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:245
> +			   if (n == 0) {

Ahh, this is the bug here.  This check should be "if (n == 0 || n > m) {".  I
only saw this bug because your test cases below made it obvious.  So, the test
cases have been proven useful already.

> JSTests/stress/replace-indexed-backreferences.js:13
> +shouldBe(string.replace(search, '$10$21$32$43'), 'abc0def1$32$43');

This looks wrong.  According to the spec, this is looking for $10, $21, $32,
and $43.  The spec for the $nn case says "If nn is 00 or nn > m, no replacement
is done."   Hence, the result should be '$10$21$32$43'.

This tells me that we have a bug in the code.  Going back above to look for the
bug ...

> JSTests/stress/replace-indexed-backreferences.js:19
> +shouldBe(search[Symbol.replace](string, '$10$21$32$43'), 'abc0def1$32$43');

Ditto.