Replace many uses of String::format with more type-safe alternatives

[Darin Adler]

Replace many uses of String::format with more type-safe alternatives

[Darin Adler]

Created attachment 357404 [details]
Patch

[EWS Watchlist]

Comment on attachment 357404 [details]
Patch

Attachment 357404 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/10419495

New failing tests:
(JS) JSTestCallTracer.cpp
(JS) JSTestCEReactions.cpp
(JS) JSTestCEReactionsStringifier.cpp
(JS) JSTestClassWithJSBuiltinConstructor.cpp
(JS) JSTestCustomConstructorWithNoInterfaceObject.cpp
(JS) JSTestActiveDOMObject.cpp
(JS) JSTestDOMJIT.cpp
(JS) JSTestEnabledBySetting.cpp
(JS) JSTestEventConstructor.cpp
(JS) JSTestEventTarget.cpp
(JS) JSTestException.cpp
(JS) JSTestGenerateIsReachable.cpp
(JS) JSTestGlobalObject.cpp
(JS) JSTestIndexedSetterNoIdentifier.cpp
(JS) JSTestIndexedSetterThrowingException.cpp
(JS) JSTestIndexedSetterWithIdentifier.cpp
(JS) JSTestInterface.cpp
(JS) JSTestInterfaceLeadingUnderscore.cpp
(JS) JSTestIterable.cpp
(JS) JSMapLike.cpp
(JS) JSTestMediaQueryListListener.cpp
(JS) JSTestNamedAndIndexedSetterNoIdentifier.cpp
(JS) JSTestNamedAndIndexedSetterThrowingException.cpp
(JS) JSTestNamedAndIndexedSetterWithIdentifier.cpp
(JS) JSTestNamedConstructor.cpp
(JS) JSTestNamedDeleterNoIdentifier.cpp
(JS) JSTestNamedDeleterThrowingException.cpp
(JS) JSTestNamedDeleterWithIdentifier.cpp
(JS) JSTestNamedDeleterWithIndexedGetter.cpp
(JS) JSTestNamedGetterCallWith.cpp
(JS) JSTestNamedGetterNoIdentifier.cpp
(JS) JSTestNamedGetterWithIdentifier.cpp
(JS) JSTestNamedSetterNoIdentifier.cpp
(JS) JSTestNamedSetterThrowingException.cpp
(JS) JSTestNamedSetterWithIdentifier.cpp
(JS) JSTestNamedSetterWithIndexedGetter.cpp
(JS) JSTestNamedSetterWithIndexedGetterAndSetter.cpp
(JS) JSTestNamedSetterWithOverrideBuiltins.cpp
(JS) JSTestNamedSetterWithUnforgableProperties.cpp
(JS) JSTestNamedSetterWithUnforgablePropertiesAndOverrideBuiltins.cpp
(JS) JSTestNode.cpp
(JS) JSTestObj.cpp
(JS) JSTestOverloadedConstructors.cpp
(JS) JSTestOverloadedConstructorsWithSequence.cpp
(JS) JSTestOverrideBuiltins.cpp
(JS) JSTestPluginInterface.cpp
(JS) JSTestPromiseRejectionEvent.cpp
(JS) JSReadOnlyMapLike.cpp
(JS) JSInterfaceName.cpp
(JS) JSTestSerialization.cpp
(JS) JSTestSerializationIndirectInheritance.cpp
(JS) JSTestSerializationInherit.cpp
(JS) JSTestSerializationInheritFinal.cpp
(JS) JSTestSerializedScriptValueInterface.cpp
(JS) JSTestStringifier.cpp
(JS) JSTestStringifierAnonymousOperation.cpp
(JS) JSTestStringifierNamedOperation.cpp
(JS) JSTestStringifierOperationImplementedAs.cpp
(JS) JSTestStringifierOperationNamedToString.cpp
(JS) JSTestStringifierReadOnlyAttribute.cpp
(JS) JSTestStringifierReadWriteAttribute.cpp
(JS) JSTestTypedefs.cpp

[Darin Adler]

Created attachment 357406 [details]
Patch

[Mark Lam]

Comment on attachment 357406 [details]
Patch

r=me

[Darin Adler]

Committed r239254: <https://trac.webkit.org/changeset/239254>

[Radar WebKit Bug Importer]

<rdar://problem/46757369>

[Matt Lewis]

This looks to have broken the windows 10 debug build:

https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/1011/steps/compile-webkit/logs/stdio

https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/997

error:

wtf\text\stringconcatenate.h(314): error C2027: use of undefined type 'WTF::StringTypeAdapter<uint64_t,void>'


revision before builds, but this one doesn't

[Matt Lewis]

Reverted r239254 for reason:

This broke the Windows 10 Debug build

Committed r239273: <https://trac.webkit.org/changeset/239273>

[Darin Adler]

That error can be fixed by adding an include of StringConcatenateNumbers.h to whatever source file failed to build, too bad we had to roll it out instead.

[Michael Catanzaro]

This is closely-related to bug #188191.

[Darin Adler]

I don’t know where to add the include because the above comment doesn’t show enough context for me to know which file failed to compile.

[Ryan Haddad]

(In reply to Darin Adler from comment #11)
> I don’t know where to add the include because the above comment doesn’t show
> enough context for me to know which file failed to compile.
It looks like the log from win-ews is still accessible:
https://webkit-queues.webkit.org/results/10421570

C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/text/StringConcatenate.h(314): error C2027: use of undefined type 'WTF::StringTypeAdapter<uint64_t,void>' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\PAL\pal\FileSizeFormatter.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\PAL\pal\PAL.vcxproj]
  C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/text/StringConcatenate.h(314): note: see declaration of 'WTF::StringTypeAdapter<uint64_t,void>' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\PAL\pal\FileSizeFormatter.cpp)
  C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/text/StringConcatenate.h(327): note: see reference to function template instantiation 'WTF::String WTF::tryMakeString<uint64_t,const char*>(uint64_t,const char *)' being compiled (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\PAL\pal\FileSizeFormatter.cpp)
  C:\cygwin\home\buildbot\WebKit\Source\WebCore\PAL\pal\FileSizeFormatter.cpp(36): note: see reference to function template instantiation 'WTF::String WTF::makeString<uint64_t,const char*>(uint64_t,const char *)' being compiled
C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/text/StringConcatenate.h(314): error C2672: 'tryMakeStringFromAdapters': no matching overloaded function found (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\PAL\pal\FileSizeFormatter.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\PAL\pal\PAL.vcxproj]

[Darin Adler]

Thanks. Looks like it’s FileSizeFormatter.cpp.

[Darin Adler]

Created attachment 360243 [details]
Patch

[Darin Adler]

Created attachment 360311 [details]
Patch

[Darin Adler]

Committed r240557: <https://trac.webkit.org/changeset/240557>

[Michael Catanzaro]

It broke the WPE/GTK debug build. I'm not sure why:

In file included from DerivedSources/ForwardingHeaders/wtf/text/AtomicString.h:364,
                 from DerivedSources/ForwardingHeaders/wtf/text/WTFString.h:667,
                 from ../../Source/WebCore/platform/network/CacheValidation.h:33,
                 from ../../Source/WebCore/loader/cache/CachedResource.h:26,
                 from ../../Source/WebCore/loader/cache/CachedImage.h:25,
                 from ../../Source/WebCore/rendering/RenderImageResource.h:28,
                 from ../../Source/WebCore/rendering/RenderImageResource.cpp:29,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp:1:
DerivedSources/ForwardingHeaders/wtf/text/StringConcatenate.h: In instantiation of ‘WTF::String WTF::tryMakeString(StringTypes ...) [with StringTypes = {const char*, int}]’:
DerivedSources/ForwardingHeaders/wtf/text/StringConcatenate.h:327:34:   required from ‘WTF::String WTF::makeString(StringTypes ...) [with StringTypes = {const char*, int}]’
../../Source/WebCore/rendering/RenderLayerCompositor.cpp:1281:84:   required from here
DerivedSources/ForwardingHeaders/wtf/text/StringConcatenate.h:314:38: error: invalid use of incomplete type ‘class WTF::StringTypeAdapter<int, void>’
     return tryMakeStringFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from DerivedSources/ForwardingHeaders/wtf/Ref.h:30,
                 from DerivedSources/ForwardingHeaders/wtf/RefPtr.h:28,
                 from DerivedSources/ForwardingHeaders/wtf/HashFunctions.h:26,
                 from DerivedSources/ForwardingHeaders/pal/SessionID.h:28,
                 from ../../Source/WebCore/platform/network/CacheValidation.h:28,
                 from ../../Source/WebCore/loader/cache/CachedResource.h:26,
                 from ../../Source/WebCore/loader/cache/CachedImage.h:25,
                 from ../../Source/WebCore/rendering/RenderImageResource.h:28,
                 from ../../Source/WebCore/rendering/RenderImageResource.cpp:29,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp:1:
DerivedSources/ForwardingHeaders/wtf/Forward.h:61:43: note: declaration of ‘class WTF::StringTypeAdapter<int, void>’
 template<typename, typename = void> class StringTypeAdapter;
                                           ^~~~~~~~~~~~~~~~~

I don't see why and templates hurt my head, so I'm going to revert just the one line change in RenderLayerCompositor.cpp. I'll also sneak in a fix for an improper string format warning that was added to this file in the past couple days. Strange coincidence that it's in the same file.

[Michael Catanzaro]

Committed r240558: <https://trac.webkit.org/changeset/240558>

[Darin Adler]

(In reply to Michael Catanzaro from comment #17)
> I don't see why

Most likely reason is the need for an include of <wtf/text/StringConcatenateNumbers.h> in RenderLayerCompositor.cpp. Would you be willing to try that and land it if it works?

[Michael Catanzaro]

OK.

Why isn't that included from StringConcatenate.h :(

[Michael Catanzaro]

Committed r240590: <https://trac.webkit.org/changeset/240590>

[Darin Adler]

(In reply to Michael Catanzaro from comment #20)
> Why isn't that included from StringConcatenate.h :(

If we prove it’s negligible slowdown to build time then we could do that to make it easier to get including right.