[libcxx-commits] [PATCH] D100722: [libc++] Fixes std::to_chars for bases != 10.

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Apr 26 12:01:04 PDT 2021


Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % nits. You probably want to ping Louis (or whoever) by name, though.



================
Comment at: libcxx/include/charconv:405
+template <typename _Tp>
+_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_INLINE_VISIBILITY int __to_chars_integral_width(_Tp __value, unsigned __base) {
+  _LIBCPP_ASSERT(__value >= 0, "The function requires a non-negative value.");
----------------
Break line after `int`.
Also, it'd be nice to use the same 4-space indents in these two new functions as are used everywhere else in the file.


================
Comment at: libcxx/include/charconv:408-410
+  unsigned __pow_2 = __base * __base;
+  unsigned __pow_3 = __pow_2 * __base;
+  unsigned __pow_4 = __pow_3 * __base;
----------------
Nit: I'd have named these `__base2`, `__base3`, `__base4` and expressed `__base4` as `__base2 * __base2`.


================
Comment at: libcxx/include/charconv:439-444
+  ptrdiff_t __diff = __last - __first;
+  int __width = __to_chars_integral_width(__value, __base);
+  if (__width > __diff)
+    return {__last, errc::value_too_large};
+
+  __last = __first + __width;
----------------
IIUC, `__diff` is `__cap` and `__width` is `__n`? I think you should do the renaming.


================
Comment at: libcxx/test/support/charconv_test_helpers.h:111
+        // doesn't modify data beyond r.ptr.
+        std::fill(buf, buf + sizeof(buf), '~');
         r = to_chars(buf, buf + sizeof(buf), v, args...);
----------------
Might be even better to initialize with
`for (int i=0; i < sizeof(buf); ++i) buf[i] = i+1;`
and check with
`for (int i=0; i < sizeof(buf); ++i) assert(buf[i] < r.ptr || buf[i] == i+1);`
or something along those lines (I didn't think too hard about it), to verify that we're not even replacing part of the buffer with some shuffled other part of the buffer, or memsetting the buffer to buf[0], or anything dumb like that.
I can't think of any plausible mechanism that would cause such a bug, though, so maybe this doesn't matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100722



More information about the libcxx-commits mailing list