[libcxx-commits] [PATCH] D128929: [libc++] Implements 128-bit support in to_chars.

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__bits:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
Can you please document our support for `__int128_t` & its unsigned counterpart under `Libc++ Extensions` in `libcxx/docs/UsingLibcxx.rst`?


================
Comment at: libcxx/include/__bits:56-58
+  return ((__x >> 64) == 0)
+           ? (64 + __builtin_clzll(static_cast<unsigned long long>(__x)))
+           : __builtin_clzll(static_cast<unsigned long long>(__x >> 64));
----------------
Mordante wrote:
> nilayvaish wrote:
> > Would this not be identified by the compiler as constexpr if _LIBCPP_STD_VER > 11?  Essentially I am trying to understand why we need #if on _LIBCPP_STD_VER.
> The above isn't valid in C++11 due to language limitations. But this isn't very readable so I added a more readable version for newer C++ versions.
IMO we should have only one version since both are equivalent. The comment can be left in place to explain why the implementation looks that way.


================
Comment at: libcxx/include/__charconv/tables.h:115
+#  ifndef _LIBCPP_HAS_NO_INT128
+_LIBCPP_CONSTEVAL __uint128_t __uint128_c(__uint128_t __a, __uint128_t __b) { return __a * __b; }
+
----------------
I don't really understand the purpose of this function. Why don't we simply multiply the operands below?


================
Comment at: libcxx/include/__charconv/tables.h:139
+    UINT64_C(10000000000000000000),
+    __uint128_c(UINT64_C(10000000000000000000), UINT64_C(10)),
+    __uint128_c(UINT64_C(10000000000000000000), UINT64_C(100)),
----------------
And so on.


================
Comment at: libcxx/include/__charconv/to_chars_base_10.h:65
 
-_LIBCPP_HIDE_FROM_ABI inline char* __append9(char* __first, uint32_t __value) noexcept {
-  return __itoa::__append8(__itoa::__append1(__first, __value / 100000000), __value % 100000000);
+// This function is used for uint32_t and uint64_t.
+template <class _Tp>
----------------
I'm not sure this comment provides a lot of value. If that's really important, we should have `enable_if` or `static_assert`. If the function really does work with pretty much any integral type, then I think the comment just doesn't have its place.


================
Comment at: libcxx/include/__charconv/to_chars_base_10.h:73
 
 // This function is used for uint32_t and uint64_t.
 template <class _Tp>
----------------
Same thing for this comment.


================
Comment at: libcxx/include/charconv:167
+    // There's always a bit set in the upper 64-bits.
+    auto __t = (128 - std::__libcpp_clz(static_cast<uint64_t>(__v >> 64))) * 1233 >> 12;
+    _LIBCPP_ASSERT(__t >= __table<>::__pow10_128_offset, "Index out of bounds");
----------------
Can you add a reference to the documentation for this trick?


================
Comment at: libcxx/include/charconv:464
 __to_chars_integral(char* __first, char* __last, _Tp __value) {
-  return __itoa::__integral<_Base>::__to_chars(__first, __last, __value);
+  return __itoa::__integral<_Base>::__to_chars(__first, __last, __value); // XXX cast to uint64_t?
 }
----------------
This is a leftover comment, I think we should be casting to `uint64_t`.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp:43
         {
-            char s[] = "0X7BAtSGHDkEIXZg ";
+			// The string has more characters than valid in an 128-bit value.
+            char s[] = "0X7BAtSGHDkEIXZgQRfYChLpOzRnM ";
----------------
Weird indentation.


================
Comment at: libcxx/test/support/charconv_test_helpers.h:153
+#ifndef TEST_HAS_NO_INT128
+    static __int128_t fromchars128(char const* p, char const* ep, int base, true_type)
+    {
----------------
That would make it clear it's a helper function only. Same comment applies to the other `false_type` version. And to `fromchars` above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128929