Created attachment 329158 [details] patch

Comment on attachment 329158 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=329158&action=review > Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:84 > + EXPECT_EQ("hello * world", makeString("hello ", static_cast<unsigned short>(42) , " world")); // Treated as a character. Note this! It's weird, but already pre-existed my patch! I'm just testing it now, and was surprised but I guess it makes sense given our odd 16-bit character types...

Attachment 329158 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 329158 [details] patch Clearing flags on attachment: 329158 Committed r225824: <https://trac.webkit.org/changeset/225824>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36009251>

Comment on attachment 329158 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=329158&action=review >> Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:84 >> + EXPECT_EQ("hello * world", makeString("hello ", static_cast<unsigned short>(42) , " world")); // Treated as a character. > > Note this! It's weird, but already pre-existed my patch! I'm just testing it now, and was surprised but I guess it makes sense given our odd 16-bit character types... I ran into this before in a patch I never landed. I think we should fix it by creating a different syntax for incorporating a single character in makeString.

(In reply to Darin Adler from comment #7) > Comment on attachment 329158 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329158&action=review > > >> Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:84 > >> + EXPECT_EQ("hello * world", makeString("hello ", static_cast<unsigned short>(42) , " world")); // Treated as a character. > > > > Note this! It's weird, but already pre-existed my patch! I'm just testing it now, and was surprised but I guess it makes sense given our odd 16-bit character types... > > I ran into this before in a patch I never landed. I think we should fix it > by creating a different syntax for incorporating a single character in > makeString. Good point! I was just talking to Sam yesterday about HexInt(...) helpers too. That's kinda the same as RawPointer(...). I filed a bug to do this: https://bugs.webkit.org/show_bug.cgi?id=180753 I'll try to get to it before end of year :-)

(In reply to JF Bastien from comment #2) > Comment on attachment 329158 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329158&action=review > > > Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:84 > > + EXPECT_EQ("hello * world", makeString("hello ", static_cast<unsigned short>(42) , " world")); // Treated as a character. > > Note this! It's weird, but already pre-existed my patch! I'm just testing it > now, and was surprised but I guess it makes sense given our odd 16-bit > character types... In WPE (and Windows given https://bugs.webkit.org/show_bug.cgi?id=187712) this seems like a dodgy test. UChar == char16_t on my machine, and apparently wchar_t on Windows. I think we're testing undefined behaviour here. Would you object to me removing this test expectation? https://bugs.webkit.org/show_bug.cgi?id=187712 explains why the template deduction breaks down in these alternative cases, it would be nice to avoid adding more platform ifdefs in the testsuites for the alternative platforms. TIA!

Proposed fix for this issue: https://bugs.webkit.org/show_bug.cgi?id=193101