Bug 204873 – [MSVC] writeNumberToBufferUnsigned is unsafe for bool type

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Fujii Hironori]

[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.

[Fujii Hironori]

Created attachment 384880 [details]
Patch

[Darin Adler]

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.

[Fujii Hironori]

Makes sense. I'm going to add static_assert at the moment.

[Fujii Hironori]

Created attachment 384988 [details]
Patch

[Darin Adler]

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

[Fujii Hironori]

I'm going to revise the message. "'bool' isn't supported"

[Fujii Hironori]

Created attachment 385129 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 385129 [details]
Patch for landing

Clearing flags on attachment: 385129

Committed r253315: <https://trac.webkit.org/changeset/253315>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57777920>