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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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> {
----------------
lntue wrote:
> Can you just include "Uint128.h" and only specialize the template for `UInt128` so that there is no need for `#ifdef` here?
I have added comments now to explain why we need both specializations. Essentially, we need both so that we can uniitest `UInt<128>` on platfroms where `UInt128` resolves to `__uint128_t`.


================
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
----------------
lntue wrote:
> Can you just include "Uint128.h" and only specialize the template for `UInt128` so that there is no need for `#ifdef` here?
Same reason as above; added comments here as well.


================
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)
----------------
lntue wrote:
> Too many tests using `uint128` implicitly I guess?
This change isn't changing the situation. The deps were incomplete previously.


================
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 <>
----------------
lntue wrote:
> Combine the specializations with `UInt128`.
Same reason as above. Added a comment now.


================
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);
----------------
lntue wrote:
> Combine the specializations with `UInt128`.
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128304