Bug 204873 - [MSVC] writeNumberToBufferUnsigned is unsafe for bool type
Summary: [MSVC] writeNumberToBufferUnsigned is unsafe for bool type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-04 21:51 PST by Fujii Hironori
Modified: 2019-12-09 17:57 PST (History)
10 users (show)

See Also:

Attachments
Patch (3.16 KB, patch)
2019-12-04 21:56 PST, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2019-12-05 19:43 PST, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch for landing (1.74 KB, patch)
2019-12-08 18:32 PST, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-12-04 21:51:19 PST 
[MSVC] writeNumberToBufferUnsigned is unsafe for bool type

MSVC reports warning for using / operator for bool type.

> warning C4804: '/': unsafe use of type 'bool' in operation

GCC and Clang doesn't report warnings for bool / 10.
https://godbolt.org/z/RjKz-m

See also Bug 204870.
Comment 1 Fujii Hironori 2019-12-04 21:56:16 PST 
Created attachment 384880 [details]
Patch
Comment 2 Darin Adler 2019-12-05 07:35:08 PST 
Comment on attachment 384880 [details]
Patch

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

I think supporting bool and turning into "0"/"1" is an accident, not intentional design. We should consider deleting the feature by disabling the template expansion instead of improving the implementation.

> Source/WTF/wtf/text/IntegerToStringConversion.h:130
> +    *destination++ = number ? '1' : '0';

No need for ++ here, if we decide to keep this code.

> Tools/TestWebKitAPI/Tests/WTF/StringConcatenate.cpp:99
> +    EXPECT_STREQ("0", makeString(false).utf8().data());
> +    EXPECT_STREQ("1", makeString(true).utf8().data());

I don’t think we should serialize as "0" and "1". Instead I would suggest "false" and "true", which is what std::ostream does.
Comment 3 Fujii Hironori 2019-12-05 19:42:28 PST 
Makes sense. I'm going to add static_assert at the moment.
Comment 4 Fujii Hironori 2019-12-05 19:43:35 PST 
Created attachment 384988 [details]
Patch
Comment 5 Darin Adler 2019-12-06 11:17:25 PST 
Comment on attachment 384988 [details]
Patch

Looks fine. I probably would have said something more like “bool not supported” rather than focusing on the implementation detail fo why this function template won’t work
Comment 6 Fujii Hironori 2019-12-08 18:28:49 PST 
I'm going to revise the message. "'bool' isn't supported"
Comment 7 Fujii Hironori 2019-12-08 18:32:42 PST 
Created attachment 385129 [details]
Patch for landing
Comment 8 Fujii Hironori 2019-12-09 17:55:44 PST 
Comment on attachment 385129 [details]
Patch for landing

Clearing flags on attachment: 385129

Committed r253315: <https://trac.webkit.org/changeset/253315>
Comment 9 Fujii Hironori 2019-12-09 17:55:48 PST 
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-12-09 17:57:15 PST 
<rdar://problem/57777920>