[webkit-reviews] review denied: [Bug 29500] Reduce memory usage of WebCore::StringImpl : [Attachment 40731] patch #4 (no longer attempting to get rid of m_sharedBufferAndFlags)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 29500: Reduce memory usage of WebCore::StringImpl
https://bugs.webkit.org/show_bug.cgi?id=29500

Attachment 40731: patch #4 (no longer attempting to get rid of
m_sharedBufferAndFlags)
https://bugs.webkit.org/attachment.cgi?id=40731&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   - Remove unnecessary m_bufferIsInternal member (saves 4 bytes).
Instead, check whether
> +	     m_data points to just pas the end of the object's members.

"pas the end"

> -    bool hasTerminatingNullCharacter() const { return
m_sharedBufferAndFlags.isFlagSet(HasTerminatingNullCharacter); }
> +    bool hasTerminatingNullCharacter() const { return
isFlagSet(HasTerminatingNullCharacter); }

This change is no longer needed.
>  
> -    bool inTable() const { return m_sharedBufferAndFlags.isFlagSet(InTable);
}
> -    void setInTable() { return m_sharedBufferAndFlags.setFlag(InTable); }
> +    bool inTable() const { return isFlagSet(InTable); }
> +    void setInTable() { setFlag(InTable); }

This change is no longer needed.

> +    bool isFlagSet(StringImplFlags) const;
> +    void setFlag(StringImplFlags);

These are no longer needed.

> +    // m_buffer is declared with zero size; the actual size is determined
when the instance
> +    // is created. It will be zero unless using an "internal buffer", in
which case m_data
> +    // will point to m_buffer and the length of m_buffer will be equal to
m_length.
> +    const UChar m_buffer[0];

Do all the compilers we need to compile with support zero-size? I ask because
I'm pretty sure it's not allowed in standard C++. In the past I've had to use a
length of 1 instead. Are there calls to new StringImpl left? If so, did you
check they allocate the correct size.

I'm going to say review- so you can remove the no-longer-needed indirection for
flag getting and setting. Otherwise this is looking fine.