Bug 194485 – Switch uses of StringBuilder with String::format for hex numbers to use HexNumber.h instead

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

Switch uses of StringBuilder with String::format for hex numbers to use HexNumber.h instead

[Darin Adler]

Created attachment 361649 [details]
Patch

[Darin Adler]

Created attachment 361650 [details]
Patch

[Darin Adler]

Adding some people to the cc list who have shown interest in this work recently. Patch is compiling and passing all tests so ready to review.

[Daniel Bates]

Comment on attachment 361650 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:438
> +            json.appendLiteral(",\"");

No need to change this. Just had a thought, raw string to avoid escaping? R"(,")" :/

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:440
> +            json.appendLiteral("\",\"");

Raw string? R"(",")"? Again, :/ (still feeling these raw strings out)

> Source/WTF/wtf/HexNumber.h:55
> +    auto unsignedNumber = static_cast<typename std::make_unsigned<NumberType>::type>(number);

Nothing to change. Just a thought, we still need typename? I thought C++14 was going to save us. Maybe that's C++17 or C++20.

> Source/WTF/wtf/HexNumber.h:70
> +    auto unsignedNumber = static_cast<typename std::make_unsigned<NumberType>::type>(number);

Ditto.

> Source/WebCore/css/parser/CSSParserToken.cpp:397
> +        break;

Nice! I don't like returning void either. Did you see <https://lists.webkit.org/pipermail/webkit-dev/2019-February/030439.html>? Your voice was noticeably absent. Opinions appreciated :)

> Source/WebKit/UIProcess/WebBackForwardList.cpp:488
> +    builder.append(m_currentIndex ? "YES" : "NO");

appendLiteral()?

[Darin Adler]

Comment on attachment 361650 [details]
Patch

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

Thank you for your review and your thoughtful comments.

>> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:438
>> +            json.appendLiteral(",\"");
> 
> No need to change this. Just had a thought, raw string to avoid escaping? R"(,")" :/

Never heard of a raw string before. I’d prefer not to use it for the first time here, but it seems like a reasonable thing to do later, revisiting all the places with \" to see if they can be made better.

>> Source/WTF/wtf/HexNumber.h:55
>> +    auto unsignedNumber = static_cast<typename std::make_unsigned<NumberType>::type>(number);
> 
> Nothing to change. Just a thought, we still need typename? I thought C++14 was going to save us. Maybe that's C++17 or C++20.

According to <https://en.cppreference.com/w/cpp/language/dependent_name> this will be resolved in C++20, which we can’t yet depend on in WebKit.

I realize on reflection we can use make_unsigned_t instead of make_unsigned. I prefer to leave this as-is now and revisit this and all other call sites where we use make_unsigned in a separate patch.

>> Source/WebCore/css/parser/CSSParserToken.cpp:397
>> +        break;
> 
> Nice! I don't like returning void either. Did you see <https://lists.webkit.org/pipermail/webkit-dev/2019-February/030439.html>? Your voice was noticeably absent. Opinions appreciated :)

I did see it. I didn’t feel that I needed to comment. Our style has been to not return void. I might make a different choice in my own project but WebKit has been consistent on this for some time and I am simply following suit. That’s what the conversation on webkit-dev also seemed to conclude without my voice.

>> Source/WebKit/UIProcess/WebBackForwardList.cpp:488
>> +    builder.append(m_currentIndex ? "YES" : "NO");
> 
> appendLiteral()?

It would need to be two calls to appendLiteral; the expression is not a literal. I prefer to leave it like this.

[Darin Adler]

Committed r241256: <https://trac.webkit.org/changeset/241256>

[Radar WebKit Bug Importer]

<rdar://problem/47955039>