Adding a user-defined literal for ASCIILiteral, like, "Hello World"_s to 1. make literal simple (instead of using ASCIILiteral("Hello World")) 2. avoid creating ASCIILiteral accidentally (If there is no way except for user-defined literal to create ASCIILiteral, we can easily ensure that ASCIILiteral's string is literal string)
Created attachment 343151 [details] Patch
Attachment 343151 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:288: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing [changelog/unwantedsecurityterms] [3] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: Do not use 'using namespace WTF::string_literals;'. [build/using_namespace] [4] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: WTF::string_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp" ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:40: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:54: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/WebDriver/WebDriverService.cpp:543: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 6 in 711 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Yusuke Suzuki from comment #0) > 2. avoid creating ASCIILiteral accidentally (If there is no way except for > user-defined literal to create ASCIILiteral, we can easily ensure that > ASCIILiteral's string is literal string) Seems like a fantastic idea. Especially this reason!
Comment on attachment 343151 [details] Patch Attachment 343151 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8274646 New failing tests: http/tests/preload/onload_event.html
Created attachment 343224 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343319 [details] Patch
Attachment 343319 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:288: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing [changelog/unwantedsecurityterms] [3] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: Do not use 'using namespace WTF::string_literals;'. [build/using_namespace] [4] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: WTF::string_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp" ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:40: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:54: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/WebDriver/WebDriverService.cpp:543: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 6 in 712 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 343321 [details] Patch
Attachment 343321 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:288: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing [changelog/unwantedsecurityterms] [3] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: Do not use 'using namespace WTF::string_literals;'. [build/using_namespace] [4] ERROR: Source/WTF/wtf/text/ASCIILiteral.h:68: WTF::string_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp" ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:40: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:54: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/WebDriver/WebDriverService.cpp:543: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 6 in 713 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 343321 [details] 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.
Comment on attachment 343321 [details] Patch Attachment 343321 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8301476 New failing tests: http/tests/preload/onload_event.html http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 343426 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 343321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343321&action=review I think removing `const char*` constructor from WTFString is practical. One downside is that ASCIILiteral does not have length information. So we cannot do `template<size_t N> const char characters[N]` thing. But WTFString's constructor does not leverage this length information. Furthermore, it is a bit unsafe since the code becomes broken if the char array includes "\0" in the middle of the array. Furthermore, if it is super important, we can do that by adding `size_t length` member since `operator ""_s` have that `size_t` information. >> 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. In our implementation, ASCIILiteral is correctly handled and it is the same efficiency to const char*. So basically using ASCIILiteral is good manner I think. And `makeString("No script for id: "_s, sourceID)` is definitely more efficient than using `+` operator. We should do that. Fixed. >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:626 >> + errorString = "No script for id: "_s + String::number(sourceID); > > Ditto. Fixed. >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:667 >> + error = "No script for id: "_s + scriptIDStr; > > Concatenation. Fixed. >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:683 >> + error = "No script for id: "_s + scriptIDStr; > > Concatenation. Fixed. >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:815 >> + errorString = "Unknown pause on exceptions mode: "_s + stringPauseState; > > Concatenation. Fixed. >> 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. Both `tryMakeString` and `makeString` uses StringTypeAdapter to write string content efficiently into the result buffer. And StringTypeAdapter already has ASCIILiteral specialization which makes the efficiency same to const char* case. So we can just use ASCIILiteral here :). >> Source/JavaScriptCore/runtime/Lookup.h:282 >> + return typeError(exec, scope, slot.isStrictMode(), ReadonlyPropertyWriteError); > > This seems to be removing a small, but valid, optimization. Or am I missing something? This is OK because ReadonlyPropertyWriteError is now ASCIILiteral! >> Source/JavaScriptCore/runtime/Lookup.h:286 >> + return typeError(exec, scope, slot.isStrictMode(), ReadonlyPropertyWriteError); > > Ditto. Ditto. >> Source/JavaScriptCore/runtime/Lookup.h:303 >> + return typeError(exec, scope, slot.isStrictMode(), ReadonlyPropertyWriteError); > > Ditto. Ditto. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:173 >> + return UnexpectedResult(makeString("WebAssembly.Module failed compiling: "_s, makeString(args)...)); > > Same makeString issue. Ditto. makeString is already efficient for ASCIILiteral :) >> Source/WTF/wtf/text/ASCIILiteral.h:46 >> + } > > What is this needed for? We would like to have some ASCIILiteral which is not basically used. For example, returned value in `RELEASE_ASSERT_NOT_REACHED()`. At that time, ASCIILiteral::null() is useful. >> Source/WTF/wtf/text/ASCIILiteral.h:54 >> + const char* m_characters; > > Extra blank line here. Removed. >> 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. Fixed. >> Source/WTF/wtf/text/ASCIILiteral.h:62 >> +} > > 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)? That's good idea! Added. >> 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. Fixed. >> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:151 >> + logDeveloperWarning("\""_s + stringValue + "\" is not a valid display mode."_s); > > Concatenation. Fixed. >> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:220 >> + logDeveloperWarning("The scope's origin of \""_s + scopeURLOrigin->toString() + "\" is different from the document's origin of \""_s + documentOrigin->toString() + "\"."_s); > > Concatenation. Fixed. >> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:89 >> + ASCIILiteral messageMiddle { ". "_s }; > > This should just be const char* because this is just used below for concatenation. Because ASCIILiteral is efficiently handled in makeString, I think it is OK. >> 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. It is OK since ASCIILiteral is efficiently handled. >> Source/WebCore/Modules/geolocation/Geolocation.cpp:507 >> + auto error = PositionError::create(PositionError::PERMISSION_DENIED, permissionDeniedErrorMessage); > > Extra space here after comma. Fixed. >> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:43 >> + : RealtimeMediaSource("WebAudio-"_s + createCanonicalUUIDString(), RealtimeMediaSource::Type::Audio, "MediaStreamAudioDestinationNode") > > Concatenation. Fixed. >> 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. `makeString("NotYetImplemented,"_s, logEnv)` makes this super efficient! >> Source/WebCore/fileapi/FileCocoa.mm:64 >> + effectiveName = (nameOverride.isNull() ? FileSystem::pathGetFileName(path) : nameOverride) + ".zip"_s; > > Concatenation. Fixed. >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2200 >> + attributes.append(Attribute(HTMLNames::hrefAttr, "tel:"_s + string)); > > Concatenation. Fixed. >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1967 >> + contextBefore = "\n "_s + contextBefore; > > Concatenation. Fixed.
Committed r233122: <https://trac.webkit.org/changeset/233122>
<rdar://problem/41394828>
Adding isASCII check in operator ""_s does not work since static_assert requires constant value. On the other hand, `constexpr` function *can* be used for non constant values. So the compile error happens.