[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 #110288|review?                     |review-
               Flag|                            |




--- Comment #17 from Darin Adler <darin at apple.com>  2011-10-09 19:57:35 PST ---
(From update of attachment 110288)
View in context: https://bugs.webkit.org/attachment.cgi?id=110288&action=review

This looks good, but is not quite ready to land. I have some comments and there is a problem because this did not build on EWS with the Windows bot.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:118
> +    AtomicString toAtomicString() const
> +    {
> +        if (!m_length)
> +            return AtomicString();
> +        if (canShrink())
> +            return AtomicString(characters(), length());
> +        if (!m_string.isNull())
> +            return AtomicString(m_string);
> +        return AtomicString(toStringPreserveCapacity());

I don’t understand the canShrink logic here. That function determines if it’s a good idea to just use the buffer rather than creating a new one since it’s already mostly full. But here, the code says “if the buffer is already full then don’t use it”, which seems like the opposite of what canShrink is supposed to mean. If this is a good algorithm, then we need a comment explaining why.

The if (!m_string.isNull) part of this function has no value; it just repeats what toStringPreserveCapacity will already do.

Your comment said that there is no easy way to optimize the case where the string is already the right size and happens to be in the table without complicating the relationship between AtomicString and StringBuilder, but that does not seem to be the case given the current form of this function.

If the AtomicString constructor had a version that took a character pointer, a length, and a StringImpl, it could have all the correct logic; StringBuilder::reifyString has nothing in it that is truly specific to StringBuilder policy.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:175
> +    // Disallow the copy constructor and the assignment operator otherwise the cost could be easily ignored.
> +    StringBuilder(const StringBuilder&);
> +    StringBuilder& operator=(const StringBuilder&);

This is a great idea. In WebKit the normal way to do this is to use WTF_NONCOPYABLE.

I would word it like this:

    // Disallow copying since it’s expensive and we don’t want code to do it by accident.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:192
> +    if (s.isEmpty() && !length)
> +        return true;
> +    if (s.length() != length)
> +        return false;

If the StringBuilder is empty, presumably the cost of calling the length function is quite small, so the first check seems to be wasteful.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:195
> +    if (s.characters() == buffer)
> +        return true;

While this condition is possible in theory, it seems it would never occur in practice. Accordingly, I suggest omitting this.

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

What’s the rationale for this change? It doesn’t seem that the copy constructor or the assignment operator are relevant here.

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