Bug 214906 – WTF::makeString() should handle enum values

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

WTF::makeString() should handle enum values.

See Bug 214756 Comment #19.

[David Kilzer (:ddkilzer)]

Created attachment 405461 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 405462 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 405463 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(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`).

[David Kilzer (:ddkilzer)]

Created attachment 405464 [details]
Patch v4

[Alex Christensen]

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.

[Alex Christensen]

When I want to print an enum value, I want to print its name, not a decimal number that has no meaning by itself.

[Darin Adler]

But in the case that drove this we wanted to print both the name and the number. That wasn’t a hypothetical.

[David Kilzer (:ddkilzer)]

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

[Sam Weinig]

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.

[Sam Weinig]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 405617 [details]
Patch v5

[David Kilzer (:ddkilzer)]

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

[Radar WebKit Bug Importer]

<rdar://problem/66552689>

[EWS]

Committed r265344: <https://trac.webkit.org/changeset/265344>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405617 [details].