[webkit-reviews] review denied: [Bug 47506] Optimize String.prototype.replace on v8-regexp : [Attachment 70481] Patch to reduce overhead.

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 47506: Optimize String.prototype.replace on v8-regexp
https://bugs.webkit.org/show_bug.cgi?id=47506

Attachment 70481: Patch to reduce overhead.
https://bugs.webkit.org/attachment.cgi?id=70481&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70481&action=review

Basic logic looks fine, but i don't really like the assignment to length inside
those if conditions, so r-

> JavaScriptCore/runtime/StringPrototype.cpp:278
> +	   if ((i < rangeCount) && (length = substringRanges[i].length)) {

This seems a little icky, i'd prefer it if we didn't assign to length inside
this condition.

You could try just:
if (i < rangeCount) {
    int length = substringRanges[i].length;
    StringImpl::copyChars(buffer + bufferPos, source.characters() +
substringRanges[i].position, length);
    bufferPos += length;
}

This allows the outer int length to be removed (so avoiding confusion over what
length is being referred to), and i would hope a 0 length string copy would
have negligible impact.

> JavaScriptCore/runtime/StringPrototype.cpp:282
> +	   if ((i < separatorCount) && (length = separators[i].length())) {

ditto.