[libc-commits] [PATCH] D110643: [libc] Add support for 128 bit ints in limits.h

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/test/utils/CPP/limits_test.cpp:15
+TEST(LlvmLibcLimitsTest, LimitsFollowSpec) {
+  ASSERT_GE(__llvm_libc::cpp::NumericLimits<int>::max(), 32767);
+  ASSERT_LE(__llvm_libc::cpp::NumericLimits<int>::min(), -32767);
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > Instead of literals, we should use the macros like `INT_MAX` etc. Then, you can make these to be equality checks. The test might seem trivial at that point - we are probably only testing for copy-paste errors. But, the macros make it more robust against platform differences.
> > right now this is comparing against the C spec, so if they fail then the platform has not implemented its integers correctly. I can change it to use the macros, but then it's not really testing anything, other than that the macros equal themselves.
> I don't disagree with your reasoning. My point is that this test is really protecting us against copy-paste errors. For example, check for `int` limits is satisfied by all other signed integer types. So, we are not really protecting ourselves from copy-paste errors. I can totally see the other way round problem also - the test itself has copy paste errors. So, up to you.
> 
> Whether the platform is implementing the C spec around integer sizes is beyond the scope of the libc I would think.
Fair enough, I've changed it to use the macros.


================
Comment at: libc/utils/CPP/Limits.h:63
+//   static constexpr __int128_t max() {
+//     return -(~__int128_t(0) + 1);
+//   }
----------------
sivachandra wrote:
> May be just `~__uint128_t(0) - 1`?
That doesn't work as int max, since it comes out to `0xfffffffffffffffffffffffffffffffe`, as opposed to the `0x7fffffffffffffffffffffffffffffff`. I've changed it to be the most logical way I can think of that doesn't rely on overflow/underflow.


================
Comment at: libc/utils/CPP/Limits.h:55
 };
+template <> class NumericLimits<__uint128_t> {
+public:
----------------
sivachandra wrote:
> abrachet wrote:
> > michaelrj wrote:
> > > sivachandra wrote:
> > > > For completeness, should we add the limits of the signed `__int128_t` type?
> > > That would be non-trivial since the test framework doesn't support `__int128_t` right now.
> > Just need to add a new Test::test instantiation for `__int128_t` like in [[ https://github.com/llvm/llvm-project/blob/c3717b6858d32d64514a187ede1a77be8ba4e542/libc/utils/UnitTest/LibcTest.cpp#L245 | LibcTest.cpp:245 ]]
> You will have to add a `describeValue` overload also like this: https://github.com/llvm/llvm-project/blob/main/libc/utils/UnitTest/LibcTest.cpp#L47
> 
> I will leave it up to you if you want to do it all in this change or just add a TODO in this change along with the commented out code.
Done, and I also had to add `__int128_t` to TypeTraits.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110643