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

Siva Chandra via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue May 10 22:57:24 PDT 2022

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

  rG LLVM Github Monorepo



More information about the libc-commits mailing list