[libcxx-commits] [PATCH] D127764: [libc++] Improve charconv base10 algorithm.

[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/__charconv/to_chars_base_10.h:28
 
-template <class _Tp>
-_LIBCPP_HIDE_FROM_ABI char* __append1(char* __buffer, _Tp __value) noexcept {
-  *__buffer = '0' + static_cast<char>(__value);
-  return __buffer + 1;
+_LIBCPP_HIDE_FROM_ABI inline char* __write1(char* __first, uint32_t __value) noexcept {
+  *__first = '0' + static_cast<char>(__value);
----------------
I think I would keep the old name `__appendN` since we are effectively still appending (based on live discussion).


================
Comment at: libcxx/include/__charconv/to_chars_base_10.h:34
+_LIBCPP_HIDE_FROM_ABI inline char* __write2(char* __first, uint32_t __value) noexcept {
+  std::memcpy(__first, &__table<>::__digits_base_10[__value * 2], 2);
+  return __first + 2;
----------------
`std::copy_n`?


================
Comment at: libcxx/include/__charconv/to_chars_base_10.h:82
+      }
+      // 100 <= __value < 10.000
+      if (__value < 1000)
----------------
I know those are just comments, however it would be more consistent with the code base to use ` ` (space) as a thousand separator instead of `.`. In the US locale, `.` is generally associated with decimals, which can be confusing.

We could also use `10'000`, that is pretty unambiguous.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127764