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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/CMakeLists.txt:63
 
+add_header_library(
+  uint128
----------------
Is this still needed?


================
Comment at: libc/src/__support/FPUtil/UInt.h:20
 
-template <size_t Bits> class UInt {
+static constexpr uint64_t Mask32 = 0xFFFFFFFF;
 
----------------
Add suffix -u/-U to the constant.


================
Comment at: libc/src/__support/FPUtil/UInt.h:20
 
-template <size_t Bits> class UInt {
+static constexpr uint64_t Mask32 = 0xFFFFFFFF;
 
----------------
lntue wrote:
> Add suffix -u/-U to the constant.
can this be all caps MASK32?


================
Comment at: libc/src/__support/FPUtil/UInt.h:40
 
   uint64_t hexval(char c) {
     uint64_t diff;
----------------
Move this out of the class.


================
Comment at: libc/src/__support/FPUtil/UInt.h:52
 
   size_t strlen(const char *s) {
     size_t len;
----------------
Move this out of the class.  You can keep them in a separate namespace of use different function name so that it won't clash with the usual strlen.


================
Comment at: libc/src/__support/FPUtil/UInt.h:60
 public:
-  UInt() { kind = Valid; }
+  constexpr UInt() {}
 
----------------
If you remove trivial implementations of the default and copy constructors, does it still work correctly?


================
Comment at: libc/src/__support/FPUtil/UInt.h:91
+  // Initialize the first word to |v| and the rest to 0.
+  constexpr UInt(uint64_t v) { init(v); }
+  /*
----------------
init method is only used here, why don't we just leave its implementation here and remove the private init method?


================
Comment at: libc/src/__support/FPUtil/UInt.h:93
+  /*
+    constexpr explicit UInt(const uint64_t data[WordCount]) {
+      for (size_t i = 0; i < WordCount; ++i)
----------------
Remove this if it's not needed.


================
Comment at: libc/src/__support/FPUtil/UInt.h:199
+    UInt<Bits> result(0);
+    result.mul(other[0]);
+    for (size_t i = 0; i < WordCount; ++i) {
----------------
This line is not needed.  `result` was 0-initialized, and multiply by other[0] would should still be 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124959