[libc-commits] [PATCH] D128304: [libc] Add Uint128 type as a fallback when __uint128_t is not available.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/CPP/Limits.h:77-88
+template <> class NumericLimits<UInt<128>> {
+public:
+  static constexpr UInt<128> max() { return ~UInt<128>(0); }
+  static constexpr UInt<128> min() { return 0; }
+};
 #ifdef __SIZEOF_INT128__
 template <> class NumericLimits<__uint128_t> {
----------------
Can you just include "Uint128.h" and only specialize the template for `UInt128` so that there is no need for `#ifdef` here?


================
Comment at: libc/src/__support/CPP/TypeTraits.h:59-62
+      IsSameV<__llvm_libc::cpp::UInt<128>, TypeNoCV>
 #ifdef __SIZEOF_INT128__
-      || IsSameV<__uint128_t, TypeNoCV> || IsSameV<__int128_t, TypeNoCV>
+      || IsSameV<__uint128_t, TypeNoCV>
 #endif
----------------
Can you just include "Uint128.h" and only specialize the template for `UInt128` so that there is no need for `#ifdef` here?


================
Comment at: libc/test/src/__support/high_precision_decimal_test.cpp:354
   EXPECT_EQ(hpd.round_to_integer_type<uint64_t>(), uint64_t(2));
 #ifdef __SIZEOF_INT128__
+  EXPECT_EQ(hpd.round_to_integer_type<UInt128>(), UInt128(2));
----------------
`#ifdef`'s are not needed in this test anymore.


================
Comment at: libc/test/src/__support/str_to_float_test.cpp:317
 }
 #elif defined(__SIZEOF_INT128__)
 TEST_F(LlvmLibcStrToFloatTest, EiselLemireFloat128Simple) {
----------------
Not needed anymore.


================
Comment at: libc/utils/UnitTest/CMakeLists.txt:8
 target_include_directories(LibcUnitTest PUBLIC ${LIBC_SOURCE_DIR})
-add_dependencies(LibcUnitTest libc.src.__support.CPP.type_traits)
+add_dependencies(LibcUnitTest libc.src.__support.CPP.type_traits libc.src.__support.CPP.uint128)
 target_link_libraries(LibcUnitTest PUBLIC libc_test_utils)
----------------
Too many tests using `uint128` implicitly I guess?


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:59-69
+#ifdef __SIZEOF_INT128__
 template <> std::string describeValue<__uint128_t>(__uint128_t Value) {
   return describeValue128(Value);
 }
 #endif
 
 template <>
----------------
Combine the specializations with `UInt128`.


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:257-271
 #ifdef __SIZEOF_INT128__
+// When builtin __uint128_t type is available, include its specialization
+// also.
 template bool test<__uint128_t>(RunContext *Ctx, TestCondition Cond,
                                 __uint128_t LHS, __uint128_t RHS,
                                 const char *LHSStr, const char *RHSStr,
                                 const char *File, unsigned long Line);
----------------
Combine the specializations with `UInt128`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128304