[libc-commits] [PATCH] D132184: [libc] add division to UInt

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/CPP/UInt.h:236
+  // cannot easily evaluate its overflow, so it returns true if it has any
+  // overflow, and relies on the client to use this information.
+  constexpr bool mul(const UInt<Bits> &other) {
----------------
What is the use case of detecting the overflow?


================
Comment at: libc/src/__support/CPP/UInt.h:293
+    }
+    if (other == UInt<Bits>(1)) {
+      return remainder;
----------------
Should we implement `operator==(uint64_t)` to make this cleaner like: `other == uint64_t(1)`?


================
Comment at: libc/src/__support/CPP/UInt.h:297
+    if (other == UInt<Bits>(0)) {
+      return optional<UInt<Bits>>();
+    }
----------------
You should be able to do return `cpp::nullopt`;


================
Comment at: libc/src/__support/CPP/UInt.h:338
+      } else {
+        // TODO(michaelrj): use builtins?
+        uint64_t num = val[i - 1];
----------------
Yes - we have https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/builtin_wrappers.h.


================
Comment at: libc/test/src/__support/uint128_test.cpp:159
+// TODO: Modulo tests
+// TODO: Exponent tests
+
----------------
We should include them in this patch? Also, if multiplication overflow is required, we should test for that also?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132184