[webkit-reviews] review denied: [Bug 66161] Investigate storing strings in 8-bit buffers when possible : [Attachment 112435] Updated patch refactored to use LChar

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 66161: Investigate storing strings in 8-bit buffers when possible
https://bugs.webkit.org/show_bug.cgi?id=66161

Attachment 112435: Updated patch refactored to use LChar
https://bugs.webkit.org/attachment.cgi?id=112435&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112435&action=review


You added LChar but didn't remove most of the static_casts. I pointed out some
of them above, but ran out of time. You need to take a sweep through and remove
them all.

> Source/JavaScriptCore/runtime/UString.h:108
> +	       return static_cast<unsigned char>(m_impl->characters8()[index]);


No need for static_cast, now that you have LChar.

> Source/JavaScriptCore/runtime/UString.h:145
>      if (rep1 == rep2) // If they're the same rep, they're equal.

This function is way too big to inline now. You should pick the right part to
inline and move the rest out of line.

> Source/JavaScriptCore/runtime/UString.h:185
> +	       if (static_cast<unsigned char>(d1[i]) != d2[i])

No need for static_cast, now that you have LChar.

> Source/JavaScriptCore/runtime/UString.h:196
> +	       if (d1[i] != static_cast<unsigned char>(d2[i]))

No need for static_cast, now that you have LChar.

> Source/JavaScriptCore/wtf/StringHasher.h:165
> +	   return static_cast<UChar>(ch);

No need for static_cast, now that you have LChar.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:222
> +	   m_copyData16[i] = static_cast<unsigned char>(m_data8[i]);

No need for static_cast, now that you have LChar.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:236
> +	       UChar c = static_cast<unsigned char>(m_data8[i]);

No need for static_cast, now that you have LChar.