[webkit-reviews] review denied: [Bug 29500] Reduce memory usage of WebCore::StringImpl : [Attachment 40324] patch #2

[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 40324: patch #2
https://bugs.webkit.org/attachment.cgi?id=40324&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    , m_buffer()

What does this line of code do? Does it have any effect at all? Can we leave it
out?

> -    // Using the constructor directly to make sure that per-thread empty
string instance isn't returned.
> -    return adoptRef(new StringImpl(m_data, m_length));
> +    // Special-case empty strings to make sure that per-thread empty string
instance isn't returned.
> +    if (m_length > 0)
> +	   return create(m_data, m_length);
> +    return adoptRef(new StringImpl());

We'd normally put the unusual case (zero-length string) first in the nested if.


I'd normally omit the () after StringImpl since it's not needed.

> -    bool startsWith(StringImpl* m_data, bool caseSensitive = true) { return
reverseFind(m_data, 0, caseSensitive) == 0; }
> +    bool startsWith(StringImpl* impl, bool caseSensitive = true) { return
reverseFind(impl, 0, caseSensitive) == 0; }

I think it's confusing to call this other string "impl". Calling it m_data was
much worse, but still, it's not the impl, it's another string!

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

I don't believe the "inline" in these is needed.

WebKit coding style requires omitting the argument name "flag" here. I also
think it's confusing that the enum type StringImplFlags is a type used not for
flags, but for the flag number of a single flag!

What's the performance impact of this?

I'm going to say review- even though my comments were minor. They should be
easy to resolve.