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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__bits:46
 
+#  ifndef _LIBCPP_HAS_NO_INT128
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
----------------
nilayvaish wrote:
> Why have space between # and ifndef?
This is our clang-format style. In some places, mainly `__config` we have a lot of nested preprocessor directives. Indenting them improves readability.


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


================
Comment at: libcxx/include/__charconv/tables.h:39
+#  ifndef _LIBCPP_HAS_NO_INT128
+  // TODO FMT Reduce the number of entries in this table.
+  static const __uint128_t __pow10_128[40];
----------------
nilayvaish wrote:
> What's FMT?
FMT is short for format, I added this for `std::format`, so I should clean this up for `std::format`.


================
Comment at: libcxx/include/__charconv/tables.h:119
+const __uint128_t __table<_Tp>::__pow10_128[40] = {
+    UINT64_C(0),
+    UINT64_C(10),
----------------
nilayvaish wrote:
> nilayvaish wrote:
> > Is it possible to use quotes (they are only allowed since C++14) after every three digits, starting from the least-significant digit?  Else, maybe we should generate these individual items using pre-processing.  I think in the current form it is hard to know if the number of digits is correct.
> Should this be 10^0 = 1?  I did notice that this matches the tables for 32 bits and 64 bits.
No it shouldn't. The length determination algorithm uses `__builtin_clzll` which requires the value to be not `0` so there the value is adjusted (orred by 1). That way a value of zero has a width of `1` and needs no further adjustment.

I agree the naming used is a bit misleading, but I decided to keep the naming already used before.


================
Comment at: libcxx/include/__charconv/tables.h:119-158
+    UINT64_C(0),
+    UINT64_C(10),
+    UINT64_C(100),
+    UINT64_C(1000),
+    UINT64_C(10000),
+    UINT64_C(100000),
+    UINT64_C(1000000),
----------------
Mordante wrote:
> nilayvaish wrote:
> > nilayvaish wrote:
> > > Is it possible to use quotes (they are only allowed since C++14) after every three digits, starting from the least-significant digit?  Else, maybe we should generate these individual items using pre-processing.  I think in the current form it is hard to know if the number of digits is correct.
> > Should this be 10^0 = 1?  I did notice that this matches the tables for 32 bits and 64 bits.
> No it shouldn't. The length determination algorithm uses `__builtin_clzll` which requires the value to be not `0` so there the value is adjusted (orred by 1). That way a value of zero has a width of `1` and needs no further adjustment.
> 
> I agree the naming used is a bit misleading, but I decided to keep the naming already used before.
It's not possible since this needs to compile in C++11. (In fact in another to_chars patch I used quotes until C++11 complained.) In the current form it's indeed looking at the pattern for a shift of one, not great. Do you have a suggestion how to do this cleanly using pre-processing; usually I feel pre-processing doesn't help to make things readable.


================
Comment at: libcxx/include/__charconv/to_chars_base_10.h:151
+  // Maximum unsigned values
+  // 64  bit                              8'446'744'073'709'551'615 (19 digits)
+  // 128 bit    340'282'366'920'938'463'463'374'607'431'768'211'455 39 digits)
----------------
nilayvaish wrote:
> I think the MSB is missing.
What do you exactly mean with "the MSB is missing"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128929