Bug 238264 – Prepare JSC for making the String(const char*) constructor explicit

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 455518 [details]
Patch

[Chris Dumez]

Created attachment 455530 [details]
Patch

[Chris Dumez]

Created attachment 455533 [details]
Patch

[Chris Dumez]

Created attachment 455534 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 455538 [details]
Patch

[Geoffrey Garen]

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?

[Chris Dumez]

Created attachment 455574 [details]
Patch

[Chris Dumez]

Committed r291779 (248807@trunk): <https://commits.webkit.org/248807@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/90738742>

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/webstorage/event_case_sensitive.html