[webkit-reviews] review granted: [Bug 217577] Cannot create StringImpl from empty string literal : [Attachment 411047] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 217577: Cannot create StringImpl from empty string literal
https://bugs.webkit.org/show_bug.cgi?id=217577

Attachment 411047: Patch

https://bugs.webkit.org/attachment.cgi?id=411047&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 411047
  --> https://bugs.webkit.org/attachment.cgi?id=411047
Patch

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:370
> +	       return jsNontrivialString(vm, emptyString());

Not sure what we really need in this unreachable place, but if we want to use
an empty string, then how about using jsEmptyString(vm)?

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:1033
> +	   String hash = emptyString();

No initializer is needed here, since String already initializes to the null
string. Note that the code below always overwrites the value of hash anyway. I
would suggest initializing to "<nil>"_s and then removing the else below, or
not initializing at all.

> Source/WTF/wtf/text/ASCIILiteral.h:68
> +    ASSERT_UNDER_CONSTEXPR_CONTEXT(n);

Nice! Does it work?

> Source/WebCore/page/NavigatorBase.cpp:92
> +	   platformName.construct(uname(&osname) >= 0 ? String(osname.sysname)
+ " "_str + String(osname.machine) : emptyString());

This is seems excessively inefficient. Using emptyString() is fine, I suppose.
But the code before should be makeString(osname.sysname, ' ', osname.machine)
so we don’t create and destroy so many temporary strings.

> Source/WebCore/platform/UserAgentQuirks.cpp:191
> +    return emptyString();

In this unreachable place it seems like we could return ASCIILiteral::null()
and the function could return ASCIILiteral. Could also return a null string
instead of an empty one, which is slightly more efficient.