Prepare JSC for making the String(const char*) constructor explicit. Making this constructor explicit helps catch at compile time cases where the ""_s prefix is missing on String literals.
Created attachment 455518 [details] Patch
Created attachment 455530 [details] Patch
Created attachment 455533 [details] Patch
Created attachment 455534 [details] Patch
Comment on attachment 455518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455518&action=review I love how this reveals the idioms used. Some comments about the less-great ones here. Another thing this points out is cases where Latin-1 may not be the right way to interpret non-ASCII characters and we might want UTF-8 instead. I like the idea of landing the obviously 100% correct _s cases first, so the biggest patch is the most obviously right. Like all the ClassInfo. > Source/JavaScriptCore/API/JSAPIGlobalObject.mm:93 > + absoluteURL = URL(URL({ }, "file://"_s), specifier); This isn’t taking advantage of our new simpler URL constructor! > Source/JavaScriptCore/API/JSValue.mm:1182 > + StructHandlers::iterator iter = structHandlers->find(String { type.get() }); Seems like an unfortunate situation where we have to make a String just to search for something. Probably fixable. > Source/JavaScriptCore/API/JSValue.mm:1220 > + StructHandlers::iterator iter = structHandlers->find(String { encodedType }); Ditto. > Source/JavaScriptCore/API/JSWrapperMap.mm:454 > + auto iter = initTable.find(String { name }); Ditto. > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:95 > + auto functionString = makeString("(function(WebInspectorAudit) { \"use strict\"; return eval(`(", String { test }.replace('`', "\\`"_s), ")`)(WebInspectorAudit); })"); Ah, make a String just to pass it to replace, yuck. > Source/JavaScriptCore/jsc.cpp:1099 > + return FileSystem::pathByAppendingComponent(String { cachePath }, makeString(source().toString().hash(), '-', filename, ".bytecode-cache")); Maybe this FileSystem function should be changed to take StringView.
(In reply to Darin Adler from comment #5) > Comment on attachment 455518 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455518&action=review > > I love how this reveals the idioms used. Some comments about the less-great > ones here. > > Another thing this points out is cases where Latin-1 may not be the right > way to interpret non-ASCII characters and we might want UTF-8 instead. > > I like the idea of landing the obviously 100% correct _s cases first, so the > biggest patch is the most obviously right. Like all the ClassInfo. > > > Source/JavaScriptCore/API/JSAPIGlobalObject.mm:93 > > + absoluteURL = URL(URL({ }, "file://"_s), specifier); > > This isn’t taking advantage of our new simpler URL constructor! > > > Source/JavaScriptCore/API/JSValue.mm:1182 > > + StructHandlers::iterator iter = structHandlers->find(String { type.get() }); > > Seems like an unfortunate situation where we have to make a String just to > search for something. Probably fixable. > > > Source/JavaScriptCore/API/JSValue.mm:1220 > > + StructHandlers::iterator iter = structHandlers->find(String { encodedType }); > > Ditto. > > > Source/JavaScriptCore/API/JSWrapperMap.mm:454 > > + auto iter = initTable.find(String { name }); > > Ditto. > > > Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:95 > > + auto functionString = makeString("(function(WebInspectorAudit) { \"use strict\"; return eval(`(", String { test }.replace('`', "\\`"_s), ")`)(WebInspectorAudit); })"); > > Ah, make a String just to pass it to replace, yuck. Yes, I am working on adding an overload that takes a StringView separately. > > > Source/JavaScriptCore/jsc.cpp:1099 > > + return FileSystem::pathByAppendingComponent(String { cachePath }, makeString(source().toString().hash(), '-', filename, ".bytecode-cache")); > > Maybe this FileSystem function should be changed to take StringView.
Created attachment 455538 [details] Patch
Comment on attachment 455538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455538&action=review r=me > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:214 > - className = thisObject->className(vm); > + className = thisObject->className(vm); oops?
Created attachment 455574 [details] Patch
Committed r291779 (248807@trunk): <https://commits.webkit.org/248807@trunk>
<rdar://problem/90738742>
Found 1 new test failure: imported/w3c/web-platform-tests/webstorage/event_case_sensitive.html