[webkit-reviews] review granted: [Bug 67081] Basic enhancements to StringBuilder : [Attachment 122500] patch v6

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

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


I cleared the commit-queue flag because the patch does not apply so the commit
queue would just fail.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:255
> +    StringImpl* impl;

I would name this “underlyingString” or “baseString” rather than “impl”.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:288
> +    if (!offset && length == r->length())
> +	   return add(r);

This should be >= rather than ==.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:85
> +	   if (!other.m_length)
> +	       return;
> +
> +	   // If we're appending to an empty string, and there is not a buffer
(reserveCapacity has not been called)
> +	   // then just retain the string.
> +	   if (!m_length && !m_buffer && !other.m_string.isNull()) {
> +	       m_string = other.m_string;
> +	       m_length = other.m_length;
> +	       return;
> +	   }
> +	   append(other.characters(), other.m_length);

Seems like there should be another blank line after the if block but before the
call to append.