[libcxx-commits] [PATCH] D103433: [libc++][format] Adds integer formatter.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__format/formatter_integer.h:111-116
+    using _To = long long;
+    if (__value < numeric_limits<_To>::min() ||
+        __value > numeric_limits<_To>::max())
+      __throw_format_error("128 bit value is outside of implemented range");
+
+    return _Base::format(static_cast<_To>(__value), __ctx);
----------------
Mordante wrote:
> vitaut wrote:
> > AFAICS `to_chars` supports 128-bit integers (https://godbolt.org/z/7GTPrcq88) or am I missing something?
> libc++ accepts 128-bit integers, but silently truncates them to 64-bit. IMO this is a bug and it's on my TODO to fix this.
> It's visible in the generated assembly, it contains a`std::__1::__itoa::__pow10_64` lookup table instead of a `std::__1::__itoa::__pow10_128`.
That is definitely a bug; see https://godbolt.org/z/j9r7YG8aq where there's a discontinuity at `base=10` (but I think all the other outputs are correct, probably?). It could be quickly patched like this:
```
diff --git a/libcxx/include/charconv b/libcxx/include/charconv
index 4a5c65df7c75..74b8993cf392 100644
--- a/libcxx/include/charconv
+++ b/libcxx/include/charconv
@@ -351,6 +351,9 @@ _LIBCPP_AVAILABILITY_TO_CHARS
 inline _LIBCPP_INLINE_VISIBILITY to_chars_result
 __to_chars_itoa(char* __first, char* __last, _Tp __value, false_type)
 {
+    if (sizeof(_Tp) > 8) {
+      return __to_chars_integral(__first, __last, __value, 10, false_type());
+
     using __tx = __itoa::__traits<_Tp>;
     auto __diff = __last - __first;
 
@@ -426,7 +429,7 @@ inline _LIBCPP_INLINE_VISIBILITY to_chars_result
 __to_chars_integral(char* __first, char* __last, _Tp __value, int __base,
                     false_type)
 {
-  if (__base == 10)
+  if (__base == 10 && sizeof(_Tp) <= 8)
     return __to_chars_itoa(__first, __last, __value, false_type());
 
   ptrdiff_t __cap = __last - __first;
```
But we'd have to add test coverage too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433