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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
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.");
----------------
Quuxplusone wrote:
> 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.
This is what clang-format makes of it. IMO if we don't like the style we should change our clang-format settings.


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


================
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;
----------------
Quuxplusone wrote:
> IIUC, `__diff` is `__cap` and `__width` is `__n`? I think you should do the renaming.
Copy-paste programming ;-) Renamed them.


================
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...);
----------------
Quuxplusone wrote:
> 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.
I also can't think of a plausible reason. However the change is trivial so I've changed it.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100722