[webkit-reviews] review granted: [Bug 98299] String::remove will convert an 8 bit string to a 16 bit string : [Attachment 166981] 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 98299: String::remove will convert an 8 bit string to a 16 bit string
https://bugs.webkit.org/show_bug.cgi?id=98299

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

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


> Source/WTF/wtf/text/WTFString.cpp:281
>      if (static_cast<unsigned>(lengthToRemove) > length() - position)
>	   lengthToRemove = length() - position;

No idea if that case is common.
If it is, shouldn't we just return a substring sharing impl here?

> Source/WTF/wtf/text/WTFString.cpp:289
> +	   RefPtr<StringImpl> newImpl =
StringImpl::createUninitialized(length() - lengthToRemove, data);
> +	   memcpy(data, characters8(), position * sizeof(LChar));
> +	   memcpy(data + position, characters8() + position + lengthToRemove,
> +	       (length() - lengthToRemove - position) * sizeof(LChar));
> +	   m_impl = newImpl.release();

I would do an inline template with that code taking CharacterType as argument.