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

[Nilay Vaish via Phabricator via libcxx-commits]

nilayvaish added inline comments.


================
Comment at: libcxx/include/__bits:46
 
+#  ifndef _LIBCPP_HAS_NO_INT128
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
----------------
Why have space between # and ifndef?


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


================
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];
----------------
What's FMT?


================
Comment at: libcxx/include/__charconv/tables.h:119
+const __uint128_t __table<_Tp>::__pow10_128[40] = {
+    UINT64_C(0),
+    UINT64_C(10),
----------------
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.


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


================
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)
----------------
I think the MSB is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128929