Bug 187712

Summary: TestWTF.WTF.StringConcatenate_Unsigned fails for unsigned short on Windows
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: Web Template FrameworkAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, don.olmstead, ews-watchlist, fujii.hironori, jfbastien, pvollan, stephan.szabo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193101
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews205 for win-future
none
Patch
none
Patch for landing
none
Patch for landing none

Ross Kirsling
Reported 2018-07-16 15:03:55 PDT
On Windows, this API test failure occurs: > TestWTF.WTF.StringConcatenate_Unsigned > > ..\..\Tools\TestWebKitAPI\Tests\WTF\StringConcatenate.cpp:84 > Value of: makeString("hello ", static_cast<unsigned short>(42) , " world") > Actual: hello 42 world > Expected: "hello * world" > Which is: 00007FFAF03F82E8 The issue seems to be a bad template deduction for StringTypeAdapter when UCHAR_TYPE is wchar_t.
Attachments
Patch (1.54 KB, patch)
2018-07-16 15:14 PDT, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews205 for win-future (12.92 MB, application/zip)
2018-07-16 23:02 PDT, EWS Watchlist 		no flags
Details
Patch (1.85 KB, patch)
2018-07-19 21:28 PDT, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch for landing (1.86 KB, patch)
2018-07-19 22:03 PDT, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch for landing (1.86 KB, patch)
2018-07-19 22:05 PDT, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Ross Kirsling
Comment 1 2018-07-16 15:14:03 PDT
Created attachment 345123 [details] Patch
Ross Kirsling
Comment 2 2018-07-16 15:21:48 PDT
The reason for #if OS(WINDOWS) here is because we need a raw `unsigned short` in order to get prioritized above the is_integral && !is_signed case, yet SFINAE only works on template params, so we can't do something like the following: > template<> > class StringTypeAdapter<unsigned short, typename std::enable_if_t<std::is_same<UChar, wchar_t>::value>> : public StringTypeAdapter<UChar, void>
Fujii Hironori
Comment 3 2018-07-16 19:58:28 PDT
See following bugs about this issue: Bug 180720 – makeString: support more integral types Bug 180753 – makeString: add single-character helpers
EWS Watchlist
Comment 4 2018-07-16 23:01:54 PDT
Comment on attachment 345123 [details] Patch Attachment 345123 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8560502 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html http/tests/security/canvas-remote-read-remote-video-localhost.html
EWS Watchlist
Comment 5 2018-07-16 23:02:05 PDT
Created attachment 345150 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Ross Kirsling
Comment 6 2018-07-17 09:17:07 PDT
(In reply to Fujii Hironori from comment #3) > See following bugs about this issue: > > Bug 180720 – makeString: support more integral types > Bug 180753 – makeString: add single-character helpers Thanks Fujii-san! JF, do you have an opinion about how this should be handled? (Am I just making Windows behavior worse so I can please a test case?)
Ross Kirsling
Comment 7 2018-07-19 10:19:04 PDT
Rereading Bug 180720 and Bug 180753, would it make the most sense to simply guard this oddly-behaving test with #if !PLATFORM(WIN) for now?
Ross Kirsling
Comment 8 2018-07-19 21:28:18 PDT
Created attachment 345424 [details] Patch
Fujii Hironori
Comment 9 2018-07-19 21:51:09 PDT
Comment on attachment 345424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345424&action=review LGTM. > Tools/ChangeLog:1 > +2018-07-19 Ross Kirsling <rkirsling@gmail.com> gmail.com
Ross Kirsling
Comment 10 2018-07-19 22:00:49 PDT
(In reply to Fujii Hironori from comment #9) > Comment on attachment 345424 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=345424&action=review > > LGTM. > > > Tools/ChangeLog:1 > > +2018-07-19 Ross Kirsling <rkirsling@gmail.com> > > gmail.com Ack, I submitted this from a different computer. Thanks for pointing that out.
Ross Kirsling
Comment 11 2018-07-19 22:03:18 PDT
Created attachment 345425 [details] Patch for landing
WebKit Commit Bot
Comment 12 2018-07-19 22:04:42 PDT
Comment on attachment 345425 [details] Patch for landing Rejecting attachment 345425 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 345425, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Hironori Fujii found in /Volumes/Data/EWS/WebKit/Tools/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/8595808
Ross Kirsling
Comment 13 2018-07-19 22:05:34 PDT
Created attachment 345426 [details] Patch for landing
WebKit Commit Bot
Comment 14 2018-07-19 23:10:41 PDT
Comment on attachment 345426 [details] Patch for landing Clearing flags on attachment: 345426 Committed r234024: <https://trac.webkit.org/changeset/234024>
WebKit Commit Bot
Comment 15 2018-07-19 23:10:43 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-07-19 23:12:15 PDT
<rdar://problem/42416633>
Note You need to log in before you can comment on or make changes to this bug.