[webkit-reviews] review granted: [Bug 73882] Add 8 bit paths for StringTypeAdapter classes : [Attachment 118450] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 73882: Add 8 bit paths for StringTypeAdapter classes
https://bugs.webkit.org/show_bug.cgi?id=73882

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118450&action=review


> Source/JavaScriptCore/runtime/UStringConcatenate.h:45
> +    bool is8Bit() { return m_string.isNull() || m_string.is8Bit(); }

I wish we would be more consistent about these functions. For immutable objects
like String I think that is8Bit and is16Bit should both return true for null
and empty strings, characters8 and characters16 could both return a suitable
pointer (if not null terminated it could even be the null pointer), and the
guideline would be that callers check for the form they prefer first. But maybe
there’s an argument for having is8Bit return false for null strings?

> Source/JavaScriptCore/runtime/UStringConcatenate.h:50
> +	   ASSERT(is8Bit());
> +	   const LChar* characters = m_string.characters8();

Why is this assertion needed? Doesn’t characters8 already do the assertion?

> Source/JavaScriptCore/runtime/UStringConcatenate.h:59
> +	   const UChar* characters = m_string.characters();
>	   for (unsigned i = 0; i < m_length; ++i)
> -	       destination[i] = m_data[i];
> +	       destination[i] = characters[i];

I think it might be better to use character8 and characters16 for this. Why
expand the original string just to write it one time?

> Source/JavaScriptCore/runtime/UStringConcatenate.h:64
> -    const UChar* m_data;
> +    const JSC::UString& m_string;
>      unsigned m_length;

Seems wrong to have m_length if we also have m_string.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:61
> +    void writeTo(LChar* destination)
> +    {
> +	   ASSERT(is8Bit());
> +	   *destination = m_buffer;
> +    }

The assertion here seems pointless.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:84
> +	   ASSERT(is8Bit());
> +	   *destination = m_buffer;

The assertion here seems pointless.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:135
> +    void writeTo(LChar* destination)
> +    {
> +	   ASSERT(is8Bit());
> +	   for (unsigned i = 0; i < m_length; ++i)
> +	       destination[i] = static_cast<LChar>(m_buffer[i]);
> +    }

Assertion here seems pointless.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:165
> +	   memcpy(destination, m_buffer, static_cast<size_t>(m_length) *
sizeof(LChar));

Why do we need that static_cast?

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:204
> +    void writeTo(LChar* destination)
> +    {
> +	   ASSERT(is8Bit());
> +	   for (unsigned i = 0; i < m_length; ++i)
> +	       destination[i] = m_buffer[i];
> +    }

How can this ever work? Seems the assertion will always fire.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:231
> +	   memcpy(destination, m_buffer, static_cast<size_t>(m_length) *
sizeof(LChar));

Again, why the cast?

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:292
> +	       unsigned char c = m_buffer[i];
> +	       destination[i] = c;

Other places in this same patch we used static_cast for this. Why should we use
assignment instead here? Why not the other places too?

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:416
> +    bool is8Bit = adapter1.is8Bit() && adapter2.is8Bit();
> +
> +    if (is8Bit) {

I don’t think this local variable is helpful.