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

[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/__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:
> 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.
Can we file a bug report to track this work item? I'm fearful we'll forget about it forever otherwise.


================
Comment at: libcxx/include/__format/formatter_integral.h:489
+  }
+#endif
+};
----------------
This block is long enough that we can't easily see what `#if` the `#endif` relates to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433