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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante 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);
----------------
Quuxplusone wrote:
> 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.
Since we've been shipping this implementation of `std::to_chars` for a while I didn't have a lot of urgency to fix it. I still want to address this issue later. I've identified more issues with `<charconv>` and I want to look at these later. For now I want to focus on the formatting library, but `<charconv>` is still high on my list. Of course if you want to implement this work around feel free to do so; I would be happy to review it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433