[webkit-reviews] review granted: [Bug 67081] Basic enhancements to StringBuilder : [Attachment 110330] Patch v5.1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 67081: Basic enhancements to StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=67081

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

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


This looks really good. I’m going to say review+ as is, but I think you should
consider the improvements I suggest in comments.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:254
> +struct SubstringInfo {

The suffix info adds nothing. I would call this just Substring, but since it’s
not using a RefPtr it probably should be named something more like
SubstringLocation.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:288
> +    SubstringInfo buffer = { impl, offset, length };
> +    return addToStringTable<SubstringInfo, SubstringTranslator>(buffer);

It’s much less expensive to compute the hash if a substring happens to be an
entire string; the hash is already pre-computed and stored. This function
should either optimize that case or require the caller to not use it in that
case.

Also, we want to use the impl and not create a new one if the offset is 0 and
then length is the string’s length

> Source/JavaScriptCore/wtf/text/StringBuilder.cpp:67
> +	   m_string = String(); // Clear the string earlier to remove the
reference to m_buffer if any.

The word “earlier” in this comment makes no sense once the code is checked in.
I think by earlier you mean “before checking the reference count of m_buffer”
but that’s not really clear.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:36
> +    // Disallow copying since it’s expensive and we don’t want code to
do it by accident.

We normally stick to ASCII for comments in our source files, so the straight
quote is better here.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:52
> -	   // If we're appending to an empty string, and there is not buffer
> -	   // (in case reserveCapacity has been called) then just retain the
> +	   // If we're appending to an empty string, and there is not a buffer
> +	   // (in case reserveCapacity has not been called) then just retain
the
>	   // string.

You should remove the words “in case” from this comment. It would be much
clearer without that.

You should rewrap this comment so it’s not three lines long with a trailing
single word. We can go wider.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:68
> +	   // If we're appending to an empty string, and there is not a buffer
> +	   // (in case reserveCapacity has not been called) then just retain
the
> +	   // string.

You should remove the words “in case” from this comment. It would be much
clearer without that.

You should rewrap this comment so it’s not three lines long with a trailing
single word. We can go wider.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:119
> +	   // If the buffer is much over-allocated, the AtomicString should
make a fit-sized copy when needed.

I think I would word the comment like this.

    // If the buffer is sufficiently over-allocated, make a new AtomicString
from a copy so its buffer is not so large.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:126
> +	   // The AtomicString will create a substring based on m_buffer when
needed.

This comment says what AtomicString should do, but the current implementation
above does not do it (see comment above).

> Source/JavaScriptCore/wtf/text/StringBuilder.h:200
> +inline bool equal(const StringBuilder& s, const UChar* buffer, unsigned
length)
> +{
> +    if (s.length() != length)
> +	   return false;
> +    if (s.characters() == buffer)
> +	   return true;
> +    return !memcmp(s.characters(), buffer, length * sizeof(UChar));
> +}

For the new 8/16-bit world we may need to rewrite this that it doesn’t use
characters() on an 8-bit string.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:202
> +inline bool operator==(const StringBuilder& a, const StringBuilder& b) {
return equal(a, b.characters(), b.length()); }

Ditto.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:204
> +inline bool operator==(const StringBuilder& a, const String& b) { return
equal(a, b.characters(), b.length()); }

Ditto.

> Source/WebCore/editing/markup.cpp:218
> -    return result.toString().replace(0, "");
> +    String string = result.toString();
> +    return string.replace(0, "");

These are awkward. Maybe changing toString to return a reference for efficiency
is not worth the inconvenience.