Bug 143550 – Cleanup some StringBuilder use

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* SUMMARY
Cleanup some StringBuilder use:

  - if all the parts are simple strings, use makeString() instead of StringBuilder
  - use makeString() instead of String::append in some cases
  - use appendLiteral and append(char) when possible

[Joseph Pecoraro]

Created attachment 250403 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 27 files


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

[Joseph Pecoraro]

Created attachment 250406 [details]
[PATCH] Proposed Fix

[Darin Adler]

Comment on attachment 250406 [details]
[PATCH] Proposed Fix

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

Nice improvements.

> Source/WebCore/platform/graphics/OpenGLShims.cpp:74
> -    String fullFunctionName(functionName);
> -    fullFunctionName.append("ARB");
> +    String fullFunctionName = makeString(functionName, "ARB");
>      target = getProcAddress(fullFunctionName.utf8().data());

This code is bogus. It converts the function name to a WTF::String, treating non-ASCII characters as Latin-1, then converts it back to a char*, treating non-ASCII characters as UTF-8. There’s also no reason to get WTF::String involved at all when we just want to concatenate 8-bit strings, but I suppose it’s OK to do so. I would probably just write this:

    string CString concatenate(const char* a, const char* b)
    {
        CString result;
        char* buffer;
        size_t sizeA = strlen(a);
        size_t sizeB = strlen(b);
        result.newUninitialized(sizeA + sizeB, buffer);
        memcpy(buffer, a, sizeA);
        memcpy(buffer + sizeA, b, sizeB);
        return result;
    }

And then I’d use it:

    target = getProcAddress(concatenate(functionName, "ARB").data());

Or we could use WTF::String in a slightly unconventional way to avoid the extra work:

    target = getProcAddress(makeString(functionName, "ARB").characters8());

Either way, I think it reads better as a one-liner without the fullFunctionName variable.

> Source/WebCore/testing/MicroTaskTest.cpp:31
> +        m_document->addConsoleMessage(MessageSource::JS, MessageLevel::Debug, makeString("MicroTask #", String::number(m_testNumber), " has run."));

A real shame to use String::number here. StringBuilder’s appendNumber is better because it doesn’t allocate and then destroy a string. Too bad we don’t have a version that works as part of a makeString, but we don’t.

> Source/WebCore/testing/MockContentFilterSettings.cpp:56
>      static LazyNeverDestroyed<String> unblockRequestURL;
>      static std::once_flag onceFlag;
>      std::call_once(onceFlag, [] {
> -        StringBuilder unblockRequestURLBuilder;
> -        unblockRequestURLBuilder.append(ContentFilter::urlScheme());
> -        unblockRequestURLBuilder.append("://");
> -        unblockRequestURLBuilder.append(unblockURLHost());
> -        unblockRequestURL.construct(unblockRequestURLBuilder.toString());
> +        unblockRequestURL.construct(makeString(ContentFilter::urlScheme(), "://", unblockURLHost()));
>      });

This is overkill. LazyNeverDestroyed and std::call_once are only needed if we need to be thread-safe, and no way a String can be thread safe, so I am pretty sure it should be a one-liner:

    static NeverDestroyed<String> unblockRequestURL = makeString(ContentFilter::urlScheme(), "://", unblockURLHost());

[Joseph Pecoraro]

Created attachment 251188 [details]
[PATCH] For Landing

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 250406 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250406&action=review
> 
> Nice improvements.
> 
> > Source/WebCore/platform/graphics/OpenGLShims.cpp:74
> > -    String fullFunctionName(functionName);
> > -    fullFunctionName.append("ARB");
> > +    String fullFunctionName = makeString(functionName, "ARB");
> >      target = getProcAddress(fullFunctionName.utf8().data());
> 
> This code is bogus. It converts the function name to a WTF::String, treating
> non-ASCII characters as Latin-1, then converts it back to a char*, treating
> non-ASCII characters as UTF-8. There’s also no reason to get WTF::String
> involved at all when we just want to concatenate 8-bit strings, but I
> suppose it’s OK to do so. I would probably just write this:
> 
>     string CString concatenate(const char* a, const char* b)
>     {
>         CString result;
>         char* buffer;
>         size_t sizeA = strlen(a);
>         size_t sizeB = strlen(b);
>         result.newUninitialized(sizeA + sizeB, buffer);
>         memcpy(buffer, a, sizeA);
>         memcpy(buffer + sizeA, b, sizeB);
>         return result;
>     }
> 
> And then I’d use it:
> 
>     target = getProcAddress(concatenate(functionName, "ARB").data());
> 
> Or we could use WTF::String in a slightly unconventional way to avoid the
> extra work:
> 
>     target = getProcAddress(makeString(functionName, "ARB").characters8());
> 
> Either way, I think it reads better as a one-liner without the
> fullFunctionName variable.

I agree. Also, when you actually read the code, it looks like there is a bug, that the reformatting makes obvious:
<https://webkit.org/b/143964> OpenGLShims appears to have a dead store if GLES2


> > Source/WebCore/testing/MockContentFilterSettings.cpp:56
> >      static LazyNeverDestroyed<String> unblockRequestURL;
> >      static std::once_flag onceFlag;
> >      std::call_once(onceFlag, [] {
> > -        StringBuilder unblockRequestURLBuilder;
> > -        unblockRequestURLBuilder.append(ContentFilter::urlScheme());
> > -        unblockRequestURLBuilder.append("://");
> > -        unblockRequestURLBuilder.append(unblockURLHost());
> > -        unblockRequestURL.construct(unblockRequestURLBuilder.toString());
> > +        unblockRequestURL.construct(makeString(ContentFilter::urlScheme(), "://", unblockURLHost()));
> >      });
> 
> This is overkill. LazyNeverDestroyed and std::call_once are only needed if
> we need to be thread-safe, and no way a String can be thread safe, so I am
> pretty sure it should be a one-liner:
> 
>     static NeverDestroyed<String> unblockRequestURL =
> makeString(ContentFilter::urlScheme(), "://", unblockURLHost());

Done.

Also removed a few now unnecessary StringBuilder includes. Letting commit-queue land so it can test other platforms.

[WebKit Commit Bot]

Comment on attachment 251188 [details]
[PATCH] For Landing

Clearing flags on attachment: 251188

Committed r183031: <http://trac.webkit.org/changeset/183031>

[Joseph Pecoraro]

Heh, weird that cq landed this with red EWS. In any case, build fix:
http://trac.webkit.org/changeset/183032