[webkit-reviews] review denied: [Bug 67081] Basic enhancements to StringBuilder : [Attachment 106970] patch: Basic enhancements to StringBuilder

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 106970: patch: Basic enhancements to StringBuilder
https://bugs.webkit.org/attachment.cgi?id=106970&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.