[libcxx-commits] [PATCH] D70631: Microsoft's floating-point to_chars powered by Ryu and Ryu Printf

[Zhihao Yuan via Phabricator via libcxx-commits]

lichray requested changes to this revision.
lichray added a comment.

I suggest moving the core parts of the implementation to one or two translation units.  The integer portion of  `<charconv>` isn't header-only either.  One part of the issue is that, if users build their project with debug, they probably don't want the whole `<charconv>` facilities to be debugable.



================
Comment at: libcxx/include/charconv:130
 {
+inline constexpr char _Charconv_digits[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e',
+    'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};
----------------
The integer portion needs to work in C++11.
We may want to use `char_format::fixed` default precision of to_chars in implementing `std::to_string` as well, but in that case, we can create some shims.


================
Comment at: libcxx/include/charconv:965
+
+        return {_First + _Len, errc{}};
+    }
----------------
It was reported some buggy gcc versions only accept `errc()`.  Worth a fix?


================
Comment at: libcxx/include/charconv:1116
+
+// The following code generated the lookup tables for the scientific exponent X. Don't remove this code.
+#if 0
----------------
Maybe move it to some compilable utils that are controlled by cmake?


================
Comment at: libcxx/include/xcharconv_ryu.h:57
+#if !defined(_LIBCPP_COMPILER_MSVC)
+inline unsigned char _BitScanForward64(unsigned long* __index, unsigned long long __mask) {
+  if (__mask == 0) {
----------------
`__traits_base::__width` also want this.
But I prefer to give it a different names because if we name these routines like this, I can foresee users pull in `<charconv>` to use MS intrinsics.


================
Comment at: libcxx/include/xcharconv_ryu.h:119
+_LIBCPP_NODISCARD_AFTER_CXX17 inline uint32_t __float_to_bits(const float __f) {
+  uint32_t __bits = 0;
+  _VSTD::memcpy(&__bits, &__f, sizeof(float));
----------------
Do they need to be initialized to 0?


================
Comment at: libcxx/include/xcharconv_ryu.h:189
+
+_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_ALWAYS_INLINE uint64_t __ryu_umul128(const uint64_t __a, const uint64_t __b, uint64_t* const __productHi) {
+  // TRANSITION, VSO-634761
----------------
IIUC Clang supports `_umul128` https://reviews.llvm.org/D25353


================
Comment at: libcxx/include/xcharconv_ryu.h:985
+inline constexpr uint64_t __FLOAT_POW5_SPLIT[47] = {
+  1152921504606846976u, 1441151880758558720u, 1801439850948198400u, 2251799813685248000u,
+  1407374883553280000u, 1759218604441600000u, 2199023255552000000u, 1374389534720000000u,
----------------
You can use `UINT64_C` to be warning-free.


================
Comment at: libcxx/include/xcharconv_ryu.h:1472
+
+    while (__output >= 10000) {
+#ifdef __clang__ // TRANSITION, LLVM-38217
----------------
Can code like this reuse integer `to_chars`?


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

https://reviews.llvm.org/D70631