[webkit-reviews] review granted: [Bug 230437] Add LLVM/FLang's int128_t implementation to WTF : [Attachment 438537] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Philip Chimento
<philip.chimento at gmail.com>'s request for review:
Bug 230437: Add LLVM/FLang's int128_t implementation to WTF
https://bugs.webkit.org/show_bug.cgi?id=230437

Attachment 438537: Patch

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




--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 438537
  --> https://bugs.webkit.org/attachment.cgi?id=438537
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438537&action=review

r=me with comments

> Source/WTF/wtf/CMakeLists.txt:106
> +    Int128.h

Add this header to xcodeproj.

> Source/WTF/wtf/CMakeLists.txt:117
> +    LeadingZeroBitCount.h

Add this header to xcodeproj.

> Source/WTF/wtf/LeadingZeroBitCount.h:41
> +static constexpr std::uint64_t deBruijn {0x07edd5e59a4e28c2};
> +static constexpr std::uint8_t mapping[64] {63, 0, 58, 1, 59, 47, 53, 2, 60,
39,
> +    48, 27, 54, 33, 42, 3, 61, 51, 37, 40, 49, 18, 28, 20, 55, 30, 34, 11,
43,
> +    14, 22, 4, 62, 57, 46, 52, 38, 26, 32, 41, 50, 36, 17, 19, 29, 10, 13,
21,
> +    56, 45, 25, 31, 35, 16, 9, 12, 44, 24, 15, 8, 23, 7, 6, 5};
> +} // namespace

You should create cpp file, and putting these constant arrays in C++ side.
Otherwise, each translation unit will materialize this duplicate array, and
enlarging binary size.

> Source/WTF/wtf/LeadingZeroBitCount.h:43
> +inline constexpr int LeadingZeroBitCount(std::uint64_t x)

Rename it to leadingZeroBitCount

> Source/WTF/wtf/LeadingZeroBitCount.h:64
> +inline constexpr int LeadingZeroBitCount(std::uint32_t x)

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:69
> +inline constexpr int LeadingZeroBitCount(std::uint16_t x)

Ditto

> Source/WTF/wtf/LeadingZeroBitCount.h:86
> +namespace detail {
> +static constexpr std::uint8_t eightBitLeadingZeroBitCount[256] {8, 7, 6, 6,
5, 5,
> +    5, 5, 4, 4, 4, 4, 4, 4, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3,
3,
> +    3, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
2,
> +    2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1,
> +    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1,
> +    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0,
0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0,
> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0};
> +}

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:88
> +inline constexpr int LeadingZeroBitCount(std::uint8_t x)

Ditto.

> Source/WTF/wtf/LeadingZeroBitCount.h:93
> +template <typename A> inline constexpr int BitsNeededFor(A x)

Rename it to bitsNeededFor.

> Tools/TestWebKitAPI/CMakeLists.txt:57
> +    Tests/WTF/Int128.cpp
>      Tests/WTF/IntegerToStringConversion.cpp
>      Tests/WTF/IteratorRange.cpp
>      Tests/WTF/JSONValue.cpp
>      Tests/WTF/LEBDecoder.cpp
> +    Tests/WTF/LeadingZeroBitCount.cpp

Add them to xcodeproj to run these tests on macOS / iOS too.