Switch uses of StringBuilder with String::format for hex numbers to use HexNumber.h instead
Created attachment 361649 [details] Patch
Created attachment 361650 [details] Patch
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.
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()?
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.
Committed r241256: <https://trac.webkit.org/changeset/241256>
<rdar://problem/47955039>