Bug 239289 – Drop inefficient String::append() overloads

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Drop inefficient String::append() overloads to push developers to use StringBuilder / makeString() instead.

[Chris Dumez]

Created attachment 457545 [details]
Patch

[Chris Dumez]

Created attachment 457547 [details]
Patch

[Chris Dumez]

Created attachment 457553 [details]
Patch

[Sam Weinig]

Comment on attachment 457553 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBKeyData.cpp:375
> +        result = makeString(StringView(result).left(147), "..."_s);

This hurts me but its just a logging string so I will not be overly upset.

> Source/WebCore/Modules/indexeddb/shared/IDBIndexInfo.cpp:65
> +    return makeString(indentString.toString(), "Index: ", m_name, " (", m_identifier, ") keyPath: ", WebCore::loggingString(m_keyPath), '\n');

This should probably be:

indentString.append("Index: ", m_name, " (", m_identifier, ") keyPath: ", WebCore::loggingString(m_keyPath), '\n');
return indentString.toString();

or just

makeString(WTF::Indentation<1> { indent }, "Index: ", m_name, " (", m_identifier, ") keyPath: ", WebCore::loggingString(m_keyPath), '\n');

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1776
> +            nodeValue = makeString(StringView(nodeValue).left(maxTextSize), ellipsisUChar);

We should probably be using ellipsisUChar everywhere we use "..." if we want to be good macOS citizens (don't ned to make that change now though).

[Darin Adler]

Comment on attachment 457553 [details]
Patch

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

Seems like the substring(0, ...) changes to left() are not closely related to removing String::append overloads. Just a separate change included in the same patch but not mention in the title.

And many of those are cases where left often allocates a new String. Some of them could be using StringView instead.

> Source/JavaScriptCore/runtime/IntlObject.cpp:961
> +        foundLocale = makeString(foundLocaleView.left(matcherResult.extensionIndex), supportedExtension.toString(), foundLocaleView.substring(matcherResult.extensionIndex));

Not clear how cheap StringBuilder::toString is. If it’s at all expensive, we could make makeString support StringBuilder directly without doing any memory allocation, although obviously that work need not be done in this patch.

> Source/WTF/wtf/text/StringBuilder.h:64
> +    void append(ASCIILiteral);

We need to move the model to where the set of things supported by StringBuilder are the same as the set supported by makeString without so much repeated code.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:329
> +    LOG(IndexedDB, "IDBCursor::setGetResult - current key %s", getResult.keyData().loggingString().left(100).utf8().data());

Here’s one example I noticed before realizing the patter than the substring/left changes were following: Can this be done with StringView instead? Why allocate the truncated string just to use it to call utf8()?

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 457553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457553&action=review
> 
> Seems like the substring(0, ...) changes to left() are not closely related
> to removing String::append overloads. Just a separate change included in the
> same patch but not mention in the title.

Yes, I got a little too excited I think :) Splitting this part out into Bug 239306 and adopting more StringView in the process.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> Yes, I got a little too excited I think :)

Oh, I understand, believe me!

[Chris Dumez]

Created attachment 457625 [details]
Patch

[Chris Dumez]

Created attachment 457626 [details]
Patch

[EWS]

Committed r292879 (249649@main): <https://commits.webkit.org/249649@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457626 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91764586>