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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
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);
----------------
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.


================
Comment at: libc/utils/CPP/Limits.h:63
+//   static constexpr __int128_t max() {
+//     return -(~__int128_t(0) + 1);
+//   }
----------------
May be just `~__uint128_t(0) - 1`?


================
Comment at: libc/utils/CPP/Limits.h:55
 };
+template <> class NumericLimits<__uint128_t> {
+public:
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110643