[webkit-reviews] review granted: [Bug 186839] [WTF] Add user-defined literal for ASCIILiteral : [Attachment 343321] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 186839: [WTF] Add user-defined literal for ASCIILiteral
https://bugs.webkit.org/show_bug.cgi?id=186839

Attachment 343321: Patch

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




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

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

As followup to this, I think we may want to remove the constructor that makes a
String from a const char* and either just omit it entirely, replace it with a
named constructor function, or perhaps make it explicit. Because there is no
real downside to using ASCIILiteral instead. Previously, I think we didn’t do
that because we didn’t want to make string literals inconvenient to use but now
it’s practical to use ASCIILiteral all the time.

We should also see about using this to replace the other similar mechanisms
like the one for constructing AtomicString if we can do so with no runtime
cost.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:523
> +	   errorString = "No script for id: "_s + String::number(sourceID);

Concatenation, so we do not need to use ASCIILiteral to make it efficient.
String concatenation is already efficient for any const char*. Also, I am not
sure the concatenation code is as efficient for ASCIILiteral as it is for const
char*; I am concerned that it might perhaps convert an ASCIILiteral to a
temporary String and then throw it away. If that’s wrong, then it’s OK, but not
important to use _s in concatenation cases.

But also in this case we should use numeric concatenation to avoid calling
String::number:

    errorString = makeString("No script for id: ", sourceID);

Might have to add an include of the numeric concatenation header to get that to
compile.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:626
> +	   errorString = "No script for id: "_s + String::number(sourceID);

Ditto.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:667
> +	   error = "No script for id: "_s + scriptIDStr;

Concatenation.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:683
> +	   error = "No script for id: "_s + scriptIDStr;

Concatenation.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:815
> +	   errorString = "Unknown pause on exceptions mode: "_s +
stringPauseState;

Concatenation.

> Source/JavaScriptCore/runtime/Lookup.cpp:37
> +	       String getterName = tryMakeString("get "_s,
String(*propertyName.publicName()));

In cases where we are using concatenation or calling makeString or
tryMakeString, I don’t think we have properly optimized ASCIILiteral, and the
code path for const char* is already pretty good. So I think we should leave
off the _s in these cases unless we are sure the ASCIILiteral version is
optimized.

> Source/JavaScriptCore/runtime/Lookup.h:282
> -	   return typeError(exec, scope, slot.isStrictMode(),
ASCIILiteral(ReadonlyPropertyWriteError));
> +	   return typeError(exec, scope, slot.isStrictMode(),
ReadonlyPropertyWriteError);

This seems to be removing a small, but valid, optimization. Or am I missing
something?

> Source/JavaScriptCore/runtime/Lookup.h:286
> -	   return typeError(exec, scope, slot.isStrictMode(),
ASCIILiteral(ReadonlyPropertyWriteError));
> +	   return typeError(exec, scope, slot.isStrictMode(),
ReadonlyPropertyWriteError);

Ditto.

> Source/JavaScriptCore/runtime/Lookup.h:303
> -    return typeError(exec, scope, slot.isStrictMode(),
ASCIILiteral(ReadonlyPropertyWriteError));
> +    return typeError(exec, scope, slot.isStrictMode(),
ReadonlyPropertyWriteError);

Ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:173
> +	   return UnexpectedResult(makeString("WebAssembly.Module failed
compiling: "_s, makeString(args)...));

Same makeString issue.

> Source/WTF/wtf/text/ASCIILiteral.h:46
> +    static constexpr ASCIILiteral null()
> +    {
> +	   return ASCIILiteral { nullptr };
> +    }

What is this needed for?

> Source/WTF/wtf/text/ASCIILiteral.h:54
> +    constexpr explicit ASCIILiteral(const char* characters) :
m_characters(characters) { }
> +
> +
> +    const char* m_characters;

Extra blank line here.

> Source/WTF/wtf/text/ASCIILiteral.h:57
> +inline namespace string_literals {

Can you name this namespace using WebKit naming style (StringLiterals) instead?
Then the style checker would not complain about it.

> Source/WTF/wtf/text/ASCIILiteral.h:62
> +constexpr ASCIILiteral operator"" _s(const char* characters, size_t)
> +{
> +    return ASCIILiteral::fromLiteralUnsafe(characters);
> +}

Can we add a constexpr function that does a static assertion that all the
characters are ASCII? Should we add a constexpr function that checks that there
are no null characters (check the size_t against the string length)?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:94
> +	  
m_consoleContext->addConsoleMessage(std::make_unique<Inspector::ConsoleMessage>
(JSC::MessageSource::Other, JSC::MessageType::Log, JSC::MessageLevel::Warning,
"Parsing application manifest "_s + m_manifestURL.string() + ": "_s +
message));

Concatenation.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:121
> +	   logDeveloperWarning("The start_url's origin of \""_s +
startURLOrigin->toString() + "\" is different from the document's origin of
\""_s + documentOrigin->toString() + "\"."_s);

Concatenation.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:151
> +    logDeveloperWarning("\""_s + stringValue + "\" is not a valid display
mode."_s);

Concatenation.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:220
> -	   logDeveloperWarning(ASCIILiteral("The scope's origin of \"") +
scopeURLOrigin->toString() + ASCIILiteral("\" is different from the document's
origin of \"") + documentOrigin->toString() + ASCIILiteral("\"."));
> +	   logDeveloperWarning("The scope's origin of \""_s +
scopeURLOrigin->toString() + "\" is different from the document's origin of
\""_s + documentOrigin->toString() + "\"."_s);

Concatenation.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:89
> +    ASCIILiteral messageMiddle { ". "_s };

This should just be const char* because this is just used below for
concatenation.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:98
> +    document->addConsoleMessage(MessageSource::Network, MessageLevel::Error,
makeString("Beacon API cannot load "_s, error.failingURL().string(),
messageMiddle, description));

Concatenation.

> Source/WebCore/Modules/geolocation/Geolocation.cpp:507
> +	   auto error = PositionError::create(PositionError::PERMISSION_DENIED,
 permissionDeniedErrorMessage);

Extra space here after comma.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:43
> +    : RealtimeMediaSource("WebAudio-"_s + createCanonicalUUIDString(),
RealtimeMediaSource::Type::Audio, "MediaStreamAudioDestinationNode")

Concatenation.

> Source/WebCore/PAL/pal/unix/LoggingUnix.cpp:45
> +	   return "NotYetImplemented,"_s + String(logEnv);

Concatenation. Could use makeString and not convert either argument to a String
or ASCIILiteral first.

> Source/WebCore/fileapi/FileCocoa.mm:64
> +    effectiveName = (nameOverride.isNull() ?
FileSystem::pathGetFileName(path) : nameOverride) + ".zip"_s;

Concatenation.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2200
> +    attributes.append(Attribute(HTMLNames::hrefAttr, "tel:"_s + string));

Concatenation.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1967
> +		       contextBefore = "\n "_s + contextBefore;

Concatenation.