[Webkit-unassigned] [Bug 67081] Basic enhancements to StringBuilder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67081


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110330|review?                     |review+
               Flag|                            |




--- Comment #23 from Darin Adler <darin at apple.com>  2011-12-22 09:10:36 PST ---
(From update of attachment 110330)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.