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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Xianzhu Wang <wangxianzhu at chromium.org>  2011-09-14 02:59:23 PST ---
(In reply to comment #6)
> (From update of attachment 106970 [details])
> 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 verified several cases and tested, and I think the original code is correct.

The cases:
1. m_string directly uses m_buffer: This only occurs when m_buffer.length() == m_length, so the append() will definitely exceed capacity. New buffer will be allocated and m_string will be set to null.

2. m_string.impl() is a substring of m_buffer, and the append() doesn't exceed capacity. As append() will only change the content after original m_length, m_string will remain unchanged.

3. m_string.impl() is a substring of m_buffer, and the append() exceeds capacity. New buffer will be allocated and m_string will be set to null.

4. All the other mutation operations like clear(), resize() will set m_string/m_buffer to null or a new value.



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

Done.

View in context: https://bugs.webkit.org/attachment.cgi?id=106970&action=review

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:40
>> +    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.

I changed 4 of them (whose first parameter is StringBuilder) to StringBuilder methods, and the other two as inline functions that call the methods.

>> 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.

Changed to 'other'.

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:116

> 
> 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.

Done by creating two versions of it. Use m_string if not null, or m_buffer (directly or create a substring of it). Extracted a private method bufferToStringImpl() for use in toAtomicString(), toAtomicStringPreserveCapacity() and reifyString().

>> 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.

I choose to make the class noncopyable as with the copy constructor people could easily ignore the cost of it.

Changed the parameter name to "other".

>> Source/JavaScriptCore/wtf/text/StringBuilder.h:184
>> +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.

Thanks!
Changed them to use WTF::equal(StringImpl*, const UChar*, unsigned length).

Will upload a new patch soon.

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