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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/FPUtil/UInt.h:20
 
-template <size_t Bits> class UInt {
+static constexpr uint64_t Mask32 = 0xFFFFFFFF;
 
----------------
lntue wrote:
> lntue wrote:
> > Add suffix -u/-U to the constant.
> can this be all caps MASK32?
Why have these been moved out of the class? They are supposed to be internal conveniences and should not be made available from a "public" namespace. If you moved them out for use in the `operator*` specialization, then if you make them them `static`, they can still live as private members.


================
Comment at: libc/src/__support/FPUtil/UInt.h:40
 
   uint64_t hexval(char c) {
     uint64_t diff;
----------------
lntue wrote:
> Move this out of the class.
The `hexval` and `strlen` methods should be removed along with the commented out constructor. Even the `InvalidHexDigit` member.


================
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.
----------------
michaelrj wrote:
> 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`
By "disabled" do you mean the conditional on `__SIZEOF_INT128__`? If yes, you should add the `else` branches for it with this specialization. To keep everything more logically organized, you should probably move the `UInt.h` header file to `src/__support/CPP`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124959