Bug 194893 - Finish removing String::format
Summary: Finish removing String::format
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on: 195141
Blocks: 30342
  Show dependency treegraph
 
Reported: 2019-02-20 21:04 PST by Darin Adler
Modified: 2019-03-01 21:45 PST (History)
5 users (show)

See Also:


Attachments
Patch (44.60 KB, patch)
2019-02-20 21:21 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (44.50 KB, patch)
2019-02-20 21:57 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (6.02 MB, application/zip)
2019-02-21 00:05 PST, Build Bot
no flags Details
Patch (44.66 KB, patch)
2019-02-21 08:25 PST, Darin Adler
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-02-20 21:04:33 PST
Finish removing String::format
Comment 1 Darin Adler 2019-02-20 21:21:39 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2019-02-20 21:57:24 PST Comment hidden (obsolete)
Comment 3 Build Bot 2019-02-21 00:05:18 PST Comment hidden (obsolete)
Comment 4 Build Bot 2019-02-21 00:05:19 PST Comment hidden (obsolete)
Comment 5 Darin Adler 2019-02-21 08:25:42 PST
Created attachment 362610 [details]
Patch
Comment 6 Darin Adler 2019-02-21 19:08:00 PST
Passing all the tests, just need a reviewer.
Comment 7 Daniel Bates 2019-02-21 23:19:43 PST
Comment on attachment 362610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362610&action=review

> Source/WebCore/ChangeLog:16
> +        (WebCore::cpuUsageString): Use makeString, FormatttedNumber, and pad.

FormatttedNumber => FormattedNumber (remove one of the t's)

> Source/WTF/wtf/Assertions.cpp:109
> +        return String();

Ok as-is. return { }?

> Source/WTF/wtf/Assertions.cpp:113
> +    unsigned len = result;

OK as-is. length?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1327
> +    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));

I'm starting to remember why I like String::format() more :| Hoping this will grow on me.

> Source/WebCore/rendering/RenderTheme.cpp:579
> +        return makeString((time < 0 ? "-" : ""), hours, ':', pad('0', 2, minutes), ':', pad('0', 2, seconds));
> +    return makeString((time < 0 ? "-" : ""), pad('0', 2, minutes), ':', pad('0', 2, seconds));

:|
Comment 8 Darin Adler 2019-02-22 09:57:02 PST
Comment on attachment 362610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362610&action=review

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1327
>> +    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));
> 
> I'm starting to remember why I like String::format() more :| Hoping this will grow on me.

I don’t consider this "preferred style", just the easiest way to convert one of the most complicated call sites, which therefore is in the very last patch of this sequence. If there were lots of call sites like this one I might feel the loss of format strings more acutely. Note that this one call site could be made a lot prettier and more elegant in multiple ways, but my goal wasn’t that; it was to get rid of the last few callers in the simplest way.
Comment 9 Darin Adler 2019-02-24 15:57:30 PST
Committed r242014: <https://trac.webkit.org/changeset/242014>
Comment 10 Radar WebKit Bug Importer 2019-02-24 15:58:23 PST
<rdar://problem/48348149>
Comment 11 Darin Adler 2019-02-25 06:44:00 PST
Just realized that actually removing String::format may again cause problems with the web inspector like it did back when we tried to fix bug 188191. I hope not!
Comment 12 Simon Fraser (smfr) 2019-02-27 16:15:57 PST
I'm getting crashes with compositing logging enabled, under:

    logString.append(makeString(pad(' ', 12 + depth * 2, hex(reinterpret_cast<uintptr_t>(&layer))), " id ", backing->graphicsLayer()->primaryLayerID(), " (", FormattedNumber::fixedWidth(absoluteBounds.x().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.y().toFloat(), 3), '-', FormattedNumber::fixedWidth(absoluteBounds.maxX().toFloat(), 3), ',', FormattedNumber::fixedWidth(absoluteBounds.maxY().toFloat(), 3), ") ", FormattedNumber::fixedWidth(backing->backingStoreMemoryEstimate() / 1024, 2), "KB"));

Not sure why yet. I just just crash in memcpy tho.
Comment 13 Simon Fraser (smfr) 2019-02-27 16:17:11 PST
    frame #0: 0x000000010d004a69 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41
  * frame #1: 0x00000005ff656697 WebCore`void WTF::StringImpl::copyCharacters<unsigned char>(destination="", source="", numCharacters=631693656) at StringImpl.h:1088:5
    frame #2: 0x00000005ff810044 WebCore`void WTF::StringTypeAdapter<WTF::HexNumberBuffer, void>::writeTo<unsigned char>(this=0x00007ffeebc8a4d0, destination="") const at HexNumber.h:104:87
    frame #3: 0x00000006027f40be WebCore`void WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>::writeTo<unsigned char>(this=0x00007ffeebc89fa0, destination="") const at StringConcatenate.h:229:37
    frame #4: 0x00000006027f2de4 WebCore`void WTF::makeStringAccumulator<unsigned char, WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<unsigned long long, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void> >(result="", adapter=StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> >, void> @ 0x00007ffeebc89fa0, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc89f90, adapters=(m_number = 39), adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc89fe0, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f80, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f70, adapters=(m_character = '-'), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f60, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f50, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a020, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc89f48, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a038) at StringConcatenate.h:257:13
    frame #5: 0x00000006027f26d5 WebCore`WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<unsigned long long, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::FormattedNumber, void>, WTF::StringTypeAdapter<char const*, void> >(adapter=StringTypeAdapter<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> >, void> @ 0x00007ffeebc8a310, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a300, adapters=(m_number = 39), adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a350, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2f0, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2e0, adapters=(m_character = '-'), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2d0, adapters=(m_character = ','), adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2c0, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a390, adapters=StringTypeAdapter<WTF::FormattedNumber, void> @ 0x00007ffeebc8a2b8, adapters=StringTypeAdapter<const char *, void> @ 0x00007ffeebc8a3a8) at StringConcatenate.h:277:9
    frame #6: 0x00000006027f2018 WebCore`WTF::String WTF::tryMakeString<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, char const*, unsigned long long, char const*, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char const*, WTF::FormattedNumber, char const*>(strings=PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> > @ 0x00007ffeebc8a4c8, strings=" id ", strings=39, strings=" (", strings=(m_buffer = "0.000", m_length = 5), strings=',', strings=(m_buffer = "622.000", m_length = 7), strings='-', strings=(m_buffer = "414.000", m_length = 7), strings=',', strings=(m_buffer = "670.000", m_length = 7), strings=") ", strings=(m_buffer = "0.00", m_length = 4), strings="KB") at StringConcatenate.h:295:12
    frame #7: 0x00000006027cdca1 WebCore`WTF::String WTF::makeString<WTF::PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer, void> >, char const*, unsigned long long, char const*, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char, WTF::FormattedNumber, char const*, WTF::FormattedNumber, char const*>(strings=PaddingSpecification<WTF::StringTypeAdapter<WTF::HexNumberBuffer> > @ 0x00007ffeebc8a828, strings=" id ", strings=39, strings=" (", strings=(m_buffer = "0.000", m_length = 5), strings=',', strings=(m_buffer = "622.000", m_length = 7), strings='-', strings=(m_buffer = "414.000", m_length = 7), strings=',', strings=(m_buffer = "670.000", m_length = 7), strings=") ", strings=(m_buffer = "0.00", m_length = 4), strings="KB") at StringConcatenate.h:301:21
    frame #8: 0x00000006027cd253 WebCore`WebCore::RenderLayerCompositor::logLayerInfo(this=0x0000000625a6e158, layer=0x0000000633593540, phase="updateBackingAndHierarchy", depth=2) at RenderLayerCompositor.cpp:1327:22
Comment 14 Simon Fraser (smfr) 2019-02-27 16:22:09 PST
(lldb) fr va m_padding.underlyingAdapter.m_buffer
(const WTF::HexNumberBuffer &) m_padding.underlyingAdapter.m_buffer = 0x00007ffeebc8aa80: {
  characters = (__elems_ = "0.000")
  length = 631693656
}
Comment 15 Simon Fraser (smfr) 2019-02-27 20:35:48 PST
I rolled this out in r242189.
Comment 16 Simon Fraser (smfr) 2019-02-27 20:38:25 PST
I filed bug 195141 on the crashing.
Comment 17 Darin Adler 2019-02-28 09:10:30 PST
Thanks, Simon. Sorry for the inconvenience. I’ll re-land this after adding more tests and fixing the crash you ran into.
Comment 18 Darin Adler 2019-03-01 21:45:16 PST
Committed r242308: <https://trac.webkit.org/changeset/242308>