[webkit-reviews] review granted: [Bug 95810] 16 bit JSRopeString up converts an 8 bit fibers to 16 bits during resolution : [Attachment 162332] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 95810: 16 bit JSRopeString up converts an 8 bit fibers to 16 bits during
resolution
https://bugs.webkit.org/show_bug.cgi?id=95810

Attachment 162332: Patch
https://bugs.webkit.org/attachment.cgi?id=162332&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162332&action=review


The patch is great. And the bench are good so let's land this :)

Please fix the characters() call and check 2 tiny things:

> Source/WTF/wtf/text/StringImpl.h:541
> +    static void copyChars(UChar* destination, const LChar* source, unsigned
numCharacters)
> +    {
> +	   if (numCharacters == 1) {
> +	       *destination = *source;
> +	       return;
> +	   }
> +	   
> +	   for (unsigned i = 0; i < numCharacters; ++i)
> +	       destination[i] = source[i];
> +    }
> +

I have the feeling the other copyChars is always inlined. Could you check and
add ALWAYS_INLINE if necessary?

I am not sure the case numCharacters == 1 helps much here because you don't
have as much overhead with short-ish strings.