Bug 214906 - WTF::makeString() should handle enum values
Summary: WTF::makeString() should handle enum values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 214756
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-28 18:31 PDT by David Kilzer (:ddkilzer)
Modified: 2020-08-06 13:09 PDT (History)
10 users (show)

See Also:


Attachments
Patch v1 (22.47 KB, patch)
2020-07-29 08:49 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (22.65 KB, patch)
2020-07-29 09:17 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (22.58 KB, patch)
2020-07-29 09:19 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (22.58 KB, patch)
2020-07-29 09:23 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (21.87 KB, patch)
2020-07-30 14:17 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-07-28 18:31:14 PDT
WTF::makeString() should handle enum values.

See Bug 214756 Comment #19.
Comment 1 David Kilzer (:ddkilzer) 2020-07-29 08:49:28 PDT
Created attachment 405461 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2020-07-29 08:53:53 PDT
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.
Comment 3 David Kilzer (:ddkilzer) 2020-07-29 08:59:18 PDT
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
Comment 4 David Kilzer (:ddkilzer) 2020-07-29 09:01:10 PDT
(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.
Comment 5 David Kilzer (:ddkilzer) 2020-07-29 09:17:19 PDT
Created attachment 405462 [details]
Patch v2
Comment 6 David Kilzer (:ddkilzer) 2020-07-29 09:19:19 PDT
Created attachment 405463 [details]
Patch v3
Comment 7 David Kilzer (:ddkilzer) 2020-07-29 09:21:23 PDT
(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`).
Comment 8 David Kilzer (:ddkilzer) 2020-07-29 09:23:34 PDT
Created attachment 405464 [details]
Patch v4
Comment 9 Alex Christensen 2020-07-29 09:42:16 PDT
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.
Comment 10 Alex Christensen 2020-07-29 09:43:50 PDT
When I want to print an enum value, I want to print its name, not a decimal number that has no meaning by itself.
Comment 11 Darin Adler 2020-07-29 09:57:48 PDT
But in the case that drove this we wanted to print both the name and the number. That wasn’t a hypothetical.
Comment 12 David Kilzer (:ddkilzer) 2020-07-29 13:37:29 PDT
(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 13 Sam Weinig 2020-07-30 08:54:16 PDT
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 14 Sam Weinig 2020-07-30 08:55:20 PDT
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.
Comment 15 David Kilzer (:ddkilzer) 2020-07-30 10:03:56 PDT
(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 16 David Kilzer (:ddkilzer) 2020-07-30 10:06:06 PDT
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.
Comment 17 David Kilzer (:ddkilzer) 2020-07-30 14:17:49 PDT
Created attachment 405617 [details]
Patch v5
Comment 18 David Kilzer (:ddkilzer) 2020-07-30 14:18:52 PDT
(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.
Comment 19 Radar WebKit Bug Importer 2020-08-04 18:32:27 PDT
<rdar://problem/66552689>
Comment 20 EWS 2020-08-06 13:09:21 PDT
Committed r265344: <https://trac.webkit.org/changeset/265344>

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