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

[Michael Jones via Phabricator via libc-commits]

michaelrj marked an inline comment as done.
michaelrj added inline comments.


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


================
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.
----------------
sivachandra wrote:
> 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`.
I've moved `UInt.h` into `CPP`, and I've added specializations to  `LibcTest`, but they're not conditional since this class is always available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124959