[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 #106970|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2011-09-13 10:22:54 PST ---
(From update of attachment 106970)
View in context: https://bugs.webkit.org/attachment.cgi?id=106970&action=review

review- because of the mistake in the == functions

I have a question about the toString and toStringPreserveCapacity functions in this class. It seems that if you do this:

    builder.toString()
    builder.append(character)
    builder.toString()

and the buffer still has additional capacity remaining, then the second toString will return an incorrect result. I think this is a basic implementation flaw in the class as it currently stands.

I also think that for efficiency toString and toStringPreserveCapacity should return const String& rather than String.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:40
> +    friend bool operator==(const String&, const StringBuilder&);
> +    friend bool operator==(const StringBuilder&, const String&);
> +    friend bool operator==(const StringBuilder&, const StringBuilder&);
> +    friend bool operator!=(const String&, const StringBuilder&);
> +    friend bool operator!=(const StringBuilder&, const String&);
> +    friend bool operator!=(const StringBuilder&, const StringBuilder&);

Only two of the functions need to be implemented as friend functions. The rest could just call the others. I’d prefer to do it that way to cut down on the number of friend functions.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:63
> +    void append(const StringBuilder& stringBuilder)

It’s confusing for a member function of class StringBuilder to have an argument named stringBuilder, since "this" is also a string builder. I suggest finding a different name for the argument.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:116
> +    AtomicString toAtomicString() const
> +    {
> +        return AtomicString(characters(), length());
> +    }

This is not good for the case where the the string is not already in the AtomicString table. In that case we would not want to always copy and allocate a new string since we may already have one ready to go. This is likely to be a quite common case and so we should think through how to make sure we have good performance in that case.

There's also a case where we have a String and it already has the flag saying it’s an atomic string. Rehashing the characters in the string just to re-find the string that we already have a reference to is not good.

Due to these issues we probably need "non-preserve-capacity" vs. "preserve-capacity" versions of this function. The fact that we have not yet optimized this is affecting the interface, not just the implementation, of this class.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:159
> +    void swap(StringBuilder& stringBuilder)

It’s good to define a swap function.

I am concerned that this class currently is allowing the compiler to generate the default copy constructor and assignment operator. Any use of these could result in two StringBuilder objects thinking they share the same m_buffer and m_bufferCharacters or even orphan m_bufferCharacters. To fix that, we need to either make the class noncopyable or implement the copy constructor. If we do implement the copy constructor then we can implement the assignment operator trivially by combining the copy constructor with swap.

I think it’s confusing for a member function of class StringBuilder to have an argument named stringBuilder, since "this" is also a string builder. I suggest finding a different name for the argument.

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

These functions will all work wrong when m_buffer->length() is longer than m_length. I think the impl() function is probably not a good idea; it’s a sort of unreliable version of toString.

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