WTF::makeString() should handle enum values. See Bug 214756 Comment #19.
Created attachment 405461 [details] Patch v1
Comment on attachment 405461 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=405461&action=review > Source/WTF/wtf/text/IntegerToStringConversion.h:88 > +template<typename CharacterType, typename BoolType, std::enable_if_t<std::is_same_v<BoolType, bool>>* = nullptr> > +inline void writeIntegerToBuffer(BoolType value, CharacterType* destination) > +{ > + return writeIntegerToBufferImpl<CharacterType, unsigned, PositiveNumber>(value ? 1 : 0, destination); > +} Probably should use uint8_t here instead of unsigned so the stack buffer in writeIntegerToBufferImpl() doesn't need to be as big. > Source/WTF/wtf/text/IntegerToStringConversion.h:124 > +template<typename BoolType, std::enable_if_t<std::is_same_v<BoolType, bool>>* = nullptr> > +inline unsigned lengthOfIntegerAsString(BoolType value) > +{ > + return lengthOfIntegerAsStringImpl<unsigned, PositiveNumber>(value ? 1 : 0); > +} I realized after posting this that I could just do `return 1;` here.
Interesting. The WinCairo compiler considers `bool` type as is_integral<> && !is_signed<>. I'm pretty sure clang didn't have an issue with this specialization. To fix, I'll have to add a `!std::is_same_v<FooInteger, bool>` constraint to the SignedInteger and UnsignedInteger templates. C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenateNumbers.h(60): error C2668: 'WTF::lengthOfIntegerAsString': ambiguous call to overloaded function C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/IntegerToStringConversion.h(135): note: could be 'unsigned int WTF::lengthOfIntegerAsString<bool,0x0>(UnsignedIntegerType)' with [ UnsignedIntegerType=bool ] C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/IntegerToStringConversion.h(121): note: or 'unsigned int WTF::lengthOfIntegerAsString<bool,0x0>(BoolType)' with [ BoolType=bool ] C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenateNumbers.h(60): note: while trying to match the argument list '(bool)' C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenateNumbers.h(60): note: while compiling class template member function 'unsigned int WTF::StringTypeAdapter<TestWebKitAPI::BoolEnum,void>::length(void) const' C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenate.h(390): note: see reference to function template instantiation 'unsigned int WTF::StringTypeAdapter<TestWebKitAPI::BoolEnum,void>::length(void) const' being compiled C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenate.h(419): note: see reference to class template instantiation 'WTF::StringTypeAdapter<TestWebKitAPI::BoolEnum,void>' being compiled C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/text/StringConcatenate.h(425): note: see reference to function template instantiation 'WTF::String WTF::tryMakeString<TestWebKitAPI::BoolEnum>(TestWebKitAPI::BoolEnum)' being compiled ..\..\Tools\TestWebKitAPI\Tests\WTF\StringConcatenate.cpp(111): note: see reference to function template instantiation 'WTF::String WTF::makeString<TestWebKitAPI::BoolEnum>(TestWebKitAPI::BoolEnum)' being compiled
(In reply to David Kilzer (:ddkilzer) from comment #3) > Interesting. The WinCairo compiler considers `bool` type as is_integral<> > && !is_signed<>. I'm pretty sure clang didn't have an issue with this > specialization. > > To fix, I'll have to add a `!std::is_same_v<FooInteger, bool>` constraint to > the SignedInteger and UnsignedInteger templates. And clang does, too. I just didn't wait for the last recompile after fixing another issue.
Created attachment 405462 [details] Patch v2
Created attachment 405463 [details] Patch v3
(In reply to David Kilzer (:ddkilzer) from comment #6) > Created attachment 405463 [details] > Patch v3 Applied the changes in Comment #2 to Patch v2 (after fixing the compiler errors for `bool`).
Created attachment 405464 [details] Patch v4
I don't think we should do this. I think your use case in Bug 214756 Comment #19 is not a good use of this and commented what we should do instead.
When I want to print an enum value, I want to print its name, not a decimal number that has no meaning by itself.
But in the case that drove this we wanted to print both the name and the number. That wasn’t a hypothetical.
(In reply to Alex Christensen from comment #10) > When I want to print an enum value, I want to print its name, not a decimal > number that has no meaning by itself. (In reply to Darin Adler from comment #11) > But in the case that drove this we wanted to print both the name and the > number. That wasn’t a hypothetical. It does have meaning because the value is logged in both os_log() and in a register using CRASH_WITH_INFO(), and it's useful to be able to match up the numeric value in both places.
Comment on attachment 405464 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=405464&action=review > Source/WTF/wtf/text/IntegerToStringConversion.h:105 > -static unsigned lengthOfNumberAsStringImpl(UnsignedIntegerType number) > +static unsigned lengthOfIntegerAsStringImpl(UnsignedIntegerType number) These length functions can probably all be constexpr. > Source/WTF/wtf/text/IntegerToStringConversion.h:138 > +template<typename UnsignedIntegerType, std::enable_if_t<std::is_integral_v<UnsignedIntegerType> && !std::is_signed_v<UnsignedIntegerType> && !std::is_same_v<UnsignedIntegerType, bool>>* = nullptr> > +inline unsigned lengthOfIntegerAsString(UnsignedIntegerType number) > { > - return lengthOfNumberAsStringImpl<UnsignedIntegerType, PositiveNumber>(number); > + return lengthOfIntegerAsStringImpl<UnsignedIntegerType, PositiveNumber>(number); > } I wonder if this would read more cleanly using if constexpr: template<typename IntegerType> inline unsigned lengthOfIntegerAsString(IntegerType integer) { static_assert(std::is_integral_v<IntegerType>); if constexpr (std::is_same_v<BoolType, bool>) return 1; if constexpr (std::is_signed_v<IntegerType>) { if (integer < 0) return lengthOfIntegerAsStringImpl<typename std::make_unsigned_t<IntegerType>, NegativeNumber>(-integer); return lengthOfIntegerAsStringImpl<typename std::make_unsigned_t<IntegerType>, PositiveNumber>(integer); } return lengthOfIntegerAsStringImpl<IntegerType, PositiveNumber>(integer); } (note, that code is untested, so may not compile). Not necessary to change to use this, just food for thought now that we have this tool.
Comment on attachment 405464 [details] Patch v4 Undoing my r+ because it looks like there is still conversation going on about this. But from a technical point of view, looks good to me.
(In reply to Sam Weinig from comment #14) > Comment on attachment 405464 [details] > Patch v4 > > Undoing my r+ because it looks like there is still conversation going on > about this. But from a technical point of view, looks good to me. Confusingly, part of the conversation is going on in Bug 214756, which inspired this fix, instead of here.
Comment on attachment 405464 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=405464&action=review >> Source/WTF/wtf/text/IntegerToStringConversion.h:138 >> } > > I wonder if this would read more cleanly using if constexpr: > > template<typename IntegerType> inline unsigned lengthOfIntegerAsString(IntegerType integer) > { > static_assert(std::is_integral_v<IntegerType>); > if constexpr (std::is_same_v<BoolType, bool>) > return 1; > if constexpr (std::is_signed_v<IntegerType>) { > if (integer < 0) > return lengthOfIntegerAsStringImpl<typename std::make_unsigned_t<IntegerType>, NegativeNumber>(-integer); > return lengthOfIntegerAsStringImpl<typename std::make_unsigned_t<IntegerType>, PositiveNumber>(integer); > } > return lengthOfIntegerAsStringImpl<IntegerType, PositiveNumber>(integer); > } > > (note, that code is untested, so may not compile). > > Not necessary to change to use this, just food for thought now that we have this tool. Nice. DIdn't think about the compiler eliminating code in the body of the method during specialization. I'll give it a try.
Created attachment 405617 [details] Patch v5
(In reply to David Kilzer (:ddkilzer) from comment #17) > Created attachment 405617 [details] > Patch v5 Applied Sam's suggestion to both WTF::writeIntegerToBuffer() and WTF::lengthOfIntegerAsString(). I think the code reads much better.
<rdar://problem/66552689>
Committed r265344: <https://trac.webkit.org/changeset/265344> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405617 [details].