Bug 192742

Summary:

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

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

WebKit Misc.

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, dbates, ews-watchlist, jlewis3, mark.lam, mcatanzaro, ryanhaddad, sam, tsavell, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=188191

Bug Depends on:

Bug Blocks:

30342

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Darin Adler

Reported 2018-12-15 10:10:51 PST

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

Attachments
Patch (70.99 KB, patch) 2018-12-15 12:18 PST, Darin Adler	no flags	Details Formatted Diff Diff
Patch (134.93 KB, patch) 2018-12-15 14:58 PST, Darin Adler	no flags	Details Formatted Diff Diff
Patch (131.51 KB, patch) 2019-01-26 11:29 PST, Darin Adler	no flags	Details Formatted Diff Diff
Patch (132.90 KB, patch) 2019-01-27 16:30 PST, Darin Adler	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2018-12-15 12:18:35 PST Comment hidden (obsolete)

Created attachment 357404 [details] Patch

EWS Watchlist

Comment 2 2018-12-15 12:23:54 PST Comment hidden (obsolete)

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

Comment 3 2018-12-15 14:58:42 PST

Created attachment 357406 [details] Patch

Mark Lam

Comment 4 2018-12-15 15:48:19 PST

Comment on attachment 357406 [details] Patch r=me

Darin Adler

Comment 5 2018-12-15 16:09:39 PST

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

Radar WebKit Bug Importer

Comment 6 2018-12-15 16:10:31 PST

<rdar://problem/46757369>

Matt Lewis

Comment 7 2018-12-17 10:39:23 PST

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

Comment 8 2018-12-17 10:45:25 PST

Reverted r239254 for reason: This broke the Windows 10 Debug build Committed r239273: <https://trac.webkit.org/changeset/239273>

Darin Adler

Comment 9 2018-12-17 11:22:43 PST

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

Comment 10 2018-12-17 14:15:18 PST

This is closely-related to bug #188191.

Darin Adler

Comment 11 2019-01-25 15:22:51 PST

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

Comment 12 2019-01-25 15:30:15 PST

(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

Comment 13 2019-01-26 11:15:32 PST

Thanks. Looks like it’s FileSizeFormatter.cpp.

Darin Adler

Comment 14 2019-01-26 11:29:18 PST Comment hidden (obsolete)

Created attachment 360243 [details] Patch

Darin Adler

Comment 15 2019-01-27 16:30:36 PST

Created attachment 360311 [details] Patch

Darin Adler

Comment 16 2019-01-27 17:56:35 PST

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

Michael Catanzaro

Comment 17 2019-01-27 19:02:32 PST

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

Comment 18 2019-01-27 19:03:35 PST

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

Darin Adler

Comment 19 2019-01-28 09:06:50 PST

(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

Comment 20 2019-01-28 09:09:50 PST

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

Michael Catanzaro

Comment 21 2019-01-28 11:09:45 PST

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

Darin Adler

Comment 22 2019-01-28 14:45:06 PST

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

Note You need to log in before you can comment on or make changes to this bug.