Bug 95502 – Replace more instances of += with StringBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Replace more instances of += with StringBuilder

[Adam Barth]

Created attachment 161566 [details]
Patch

[WebKit Review Bot]

Attachment 161566 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSGradientValue.cpp:491:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSGradientValue.cpp:666:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 161566 [details]
Patch

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

I suspect the InspectorFileSystemAgent.cpp change is technically incorrect although unlikely to be a practical problem. Should be easy enough to fix.

> Source/WebCore/css/CSSTimingFunctionValue.cpp:50
> +    return "cubic-bezier(" +
> +        String::number(m_x1) + ", " +
> +        String::number(m_y1) + ", " +
> +        String::number(m_x2) + ", " +
> +        String::number(m_y2) + ")";

Our style is to put the + characters at the starts of the lines rather than the ends.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:584
> +    String result = decoder->decode(static_cast<char*>(buffer->data()), buffer->byteLength()) + decoder->flush();

Please don’t make this change. I believe C++ does not guarantee order of evaluation of arguments. So there is a chance that flush can be called here before decode.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:811
> +    styleSheetText.append(text);

Would be be better to construct styleSheetText with an initial value rather than using append here?

[Adam Barth]

> > Source/WebCore/inspector/InspectorStyleSheet.cpp:811
> > +    styleSheetText.append(text);
> 
> Would be be better to construct styleSheetText with an initial value rather than using append here?

I don't think StringBuilder allows that:
http://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/StringBuilder.h#L40

However, I've moved the declaration closer to the first append so it looks less goofy.

[Adam Barth]

Created attachment 161572 [details]
Patch for landing

[Adam Barth]

I fixed the other two things.  Thanks for the review!

[Benjamin Poulain]

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

A few comments in addition to Darin's:

> Source/WebCore/ChangeLog:11
> +        replace these inefficient patterns with StringBuilder.

replace -> replaced.

> Source/WebCore/css/CSSFunctionValue.cpp:50
> -        result += m_args->cssText();
> -    result += ")";
> -    return result;
> +        result.append(cssText());

You change from m_args->cssText() to cssText().

> Source/WebCore/css/CSSPrimitiveValue.cpp:951
> +            text = "counter(" + String(m_value.string) + ")";

We should fix that in WTF. Having an adapter for StringImpl.

")" -> ')'

> Source/WebCore/css/CSSTimingFunctionValue.cpp:50
> +    return "cubic-bezier(" +
> +        String::number(m_x1) + ", " +
> +        String::number(m_y1) + ", " +
> +        String::number(m_x2) + ", " +
> +        String::number(m_y2) + ")";

The ident is a little weird.

If this is a hot spot, we can do faster with appendNumber() on a Vector<char>. I doubt that's a hotspot though :)

> Source/WebCore/css/CSSTimingFunctionValue.cpp:60
> +    return "steps(" + String::number(m_steps) + ", " + (m_stepAtStart ? "start" : "end") + ")";

")" -> ')'

> Source/WebCore/fileapi/BlobURL.cpp:75
> +    String urlString = "blob:" + encodeWithURLEscapeSequences(originString) + "/" + createCanonicalUUIDString();

"/" -> '/'

> Source/WebCore/inspector/InspectorCSSAgent.cpp:329
> -            m_oldText += ";";
> +            m_oldText.append(";");

Note that those lines are equivalent.

You can chance ";" -> ';' to avoid strlen().

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:258
> -    String message = ASCIILiteral("Failed to load resource");
> -    if (!error.localizedDescription().isEmpty())
> -        message += ": " + error.localizedDescription();
> +    StringBuilder message;
> +    message.appendLiteral("Failed to load resource");
> +    if (!error.localizedDescription().isEmpty()) {
> +        message.append(' ');
> +        message.append(error.localizedDescription());
> +    }

The ':' disappeared (": " -> ' ').

> Source/WebCore/inspector/InspectorStyleSheet.cpp:804
> +    StringBuilder styleSheetText;

You could move this after the if() for clarity.

[Adam Barth]

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:329
> > -            m_oldText += ";";
> > +            m_oldText.append(";");
> 
> Note that those lines are equivalent.

Yes.  However, changing this to call append rather than operator+= will let me remove operator+= from WTFString.h.  :)

(I'll address the other comments before landing.)

[Eric Seidel (no email)]

Comment on attachment 161572 [details]
Patch for landing

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:585
>      String result = decoder->decode(static_cast<char*>(buffer->data()), buffer->byteLength());
> -    result += decoder->flush();
> +    result.append(decoder->flush());

Couldn't we just change this to be String result = decoder->decode() + decoder->flush()?

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 161572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161572&action=review
> 
> > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:585
> >      String result = decoder->decode(static_cast<char*>(buffer->data()), buffer->byteLength());
> > -    result += decoder->flush();
> > +    result.append(decoder->flush());
> 
> Couldn't we just change this to be String result = decoder->decode() + decoder->flush()?

I guess that's no fewer mallocs.

[Adam Barth]

(In reply to comment #9)
> (From update of attachment 161572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161572&action=review
> 
> > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:585
> >      String result = decoder->decode(static_cast<char*>(buffer->data()), buffer->byteLength());
> > -    result += decoder->flush();
> > +    result.append(decoder->flush());
> 
> Couldn't we just change this to be String result = decoder->decode() + decoder->flush()?

I used that idiom in a previous patch and Darin Adler commented that the order of evaluation is undefined, which could cause problems.

[Adam Barth]

> We should fix that in WTF. Having an adapter for StringImpl.

I'm going to leave that for a future patch because I don't want to screw up the StringAdaptor implementation.

[Benjamin Poulain]

(In reply to comment #12)
> > We should fix that in WTF. Having an adapter for StringImpl.
> 
> I'm going to leave that for a future patch because I don't want to screw up the StringAdaptor implementation.

That was just a note. I also think that should be in a separate patch.

[Adam Barth]

Created attachment 161592 [details]
Patch for landing

[Adam Barth]

Comment on attachment 161592 [details]
Patch for landing

Looks like I might have broken fast/css/remove-shorthand.html and fast/backgrounds/repeat/parsing-background-repeat.html

[Adam Barth]

I undid the changes to Source/WebCore/css/StylePropertySet.cpp and it fixed the issue.  I'm going to land the rest of the patch and return to that file in another bug.

[Adam Barth]

Committed r127224: <http://trac.webkit.org/changeset/127224>

[Benjamin Poulain]

Comment on attachment 161592 [details]
Patch for landing

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

> Source/WebCore/css/StylePropertySet.cpp:421
> -    return res;
> +    return result.toString();

I think this could be the problem, it should be:
    return res.isEmpty() ? String() : res.toString();

StringBuilder::toString() return an emptyString if empty. The previous code would return a null string.

[Darin Adler]

Comment on attachment 161592 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:18
> +        We can make cssText() more efficient by passing a single StringBuilder
> +        instance along to the recursive calls, but I've left that for a later
> +        patch.

I think the way to go here is to make a new function appendCSSText(StringBuilder&) const, and then have cssText functions all just be wrappers that call appendCSSText.

[Adam Barth]

Yes, makes sense.