[libc-commits] [PATCH] D124959: [libc] add uint128 implementation

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/UInt128.h:87
+  UInt128 operator>>(const int amount) {
+    uint64_t new_low = (low >> amount) | (high << (BITS_IN_UINT64 - amount));
+    uint64_t new_high = (high >> amount);
----------------
lntue wrote:
> `BITS_IN_UINT64` is `unsigned`, mixing with signed `amount` might trigger UB / warnings.
it theoretically could, although the bigger problem was the algorithm being wrong for numbers above 64. I've fixed both shifts.


================
Comment at: libc/test/src/__support/uint128_test.cpp:13
+
+// while I could use "ASSERT_TRUE(a == b)", this will tell me what the values
+// actually are.
----------------
lntue wrote:
> You should be able to add specialization for UInt128 in `LibcTest.cpp` and use ASSERT_EQ, EXPECT_EQ as usual.  Also disable the `__uint128_t` variant over there when it's not defined.
I could make `ASSERT_EQ` support my custom UInt128, but that would add a dependency for effectively no gain, since UInt128 shouldn't be called by name in any other test file. Also, the `__uint128_t` code is already disabled for `LibcTest.cpp`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124959/new/

https://reviews.llvm.org/D124959