[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);
----------------
ldionne wrote:
> 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.
I've marked it as TODO's in the format related files, for example line 151 in this file
`// TODO FMT Implement full 128 bit support.`

So I don't fear we'll forget it. I don't consider format fully implemented before these `TODO FMT`s are resolved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433