[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:
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110643