[webkit-reviews] review granted: [Bug 214906] WTF::makeString() should handle enum values : [Attachment 405464] Patch v4

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 214906: WTF::makeString() should handle enum values
https://bugs.webkit.org/show_bug.cgi?id=214906

Attachment 405464: Patch v4

https://bugs.webkit.org/attachment.cgi?id=405464&action=review




--- Comment #13 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 405464
  --> https://bugs.webkit.org/attachment.cgi?id=405464
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.