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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/UInt128.h:41
+
+  UInt128 operator+(const UInt128 other) {
+    uint64_t new_low, new_high;
----------------
`other` should either be `UInt128` or `const UInt128&`.  You can also add `const` after the declaration:
`UInt128 operator+(UInt128 other) const {`.  Same for `operator*`.


================
Comment at: libc/src/__support/UInt128.h:80
+
+  UInt128 operator<<(const int amount) {
+    uint64_t new_low = low << amount;
----------------
Drop the `const` for `amount` and add `const` at the end:
`UInt128 operator<<(int amount) const {`.  Same for other operators.


================
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);
----------------
`BITS_IN_UINT64` is `unsigned`, mixing with signed `amount` might trigger UB / warnings.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124959