[webkit-reviews] review granted: [Bug 194485] Switch uses of StringBuilder with String::format for hex numbers to use HexNumber.h instead : [Attachment 361650] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 194485: Switch uses of StringBuilder with String::format for hex numbers to
use HexNumber.h instead
https://bugs.webkit.org/show_bug.cgi?id=194485

Attachment 361650: Patch

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




--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 361650
  --> https://bugs.webkit.org/attachment.cgi?id=361650
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()?