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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Wow, this is great!

Looking forward to reading through this properly but, here are a few comments right off the bat.

I suspect that libc++ will want `__double_lowercase_ugly` but, I would let others chime in before you go to a lot of work to change things. The headers should probably also have `__double_underscores`. Also, here are some docs <https://github.com/llvm-mirror/libcxx/blob/master/NOTES.TXT#L19-L29> on adding a new header to libc++.



================
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'};
----------------
Libc++ usually uses the format `__charconv_digits`.


================
Comment at: libcxx/include/charconv:626
+template <class _Ty>
+constexpr const _Ty& _Min_value(const _Ty& _Left, const _Ty& _Right) noexcept {
+  return _Right < _Left ? _Right : _Left;
----------------
Why not `std::min`?


================
Comment at: libcxx/include/charconv:631
+_LIBCPP_END_NAMESPACE_STD
+#include <xcharconv_ryu.h>
+_LIBCPP_BEGIN_NAMESPACE_STD
----------------
Can this go at the top of the file?


================
Comment at: libcxx/include/charconv:638
+        0>
+_LIBCPP_NODISCARD_AFTER_CXX17 /*constexpr*/ _To _Bit_cast(const _From& _From_obj) noexcept {
+    _To _To_obj; // assumes default-init
----------------
Not a problem with this patch, but, it might be good to implement `bit_cast` and `__libcpp_bitcast` while we wait for the builtin constexpr one. I can work on a patch.


================
Comment at: libcxx/include/charconv:710
+    // double explicitly stores 52 fraction bits. 52 / 4 == 13, which is 13 hexits.
+    constexpr int _Full_precision         = is_same_v<_Floating, float> ? 6 : 13;
+    constexpr int _Adjusted_explicit_bits = _Full_precision * 4;
----------------
Can you use `_IsSame` so we can benefit from the builtin?


================
Comment at: libcxx/include/charconv:733
+
+    if constexpr (is_same_v<_Floating, float>) {
+        _Adjusted_mantissa = _Ieee_mantissa << 1; // align to hexit boundary (23 isn't divisible by 4)
----------------
Ditto.


================
Comment at: libcxx/include/xcharconv_ryu.h:74
+
+// clang-format off
+// vvvvvvvvvv DERIVED FROM common.h vvvvvvvvvv
----------------
I don't think we need this (no one should run clang-format on all of libc++).


================
Comment at: libcxx/include/xcharconv_ryu.h:147
+#ifdef _M_X64
+#define _LIBCPP_INTRINSIC128 1
+_LIBCPP_NODISCARD_AFTER_CXX17 inline uint64_t __ryu_umul128(const uint64_t __a, const uint64_t __b, uint64_t* const __productHi) {
----------------
Maybe this should go in `__config`?


================
Comment at: libcxx/include/xcharconv_ryu.h:193
+  // The casts here help MSVC to avoid calls to the __allmul library function.
+  const uint32_t __aLo = static_cast<uint32_t>(__a);
+  const uint32_t __aHi = static_cast<uint32_t>(__a >> 32);
----------------
Is it well-defined behavior to cast from a larger integer to a smaller one? 


================
Comment at: libcxx/include/xcharconv_ryu.h:224
+  _LIBCPP_ASSERT(__dist < 64, "");
+#ifdef _WIN64
+  _LIBCPP_ASSERT(__dist > 0, "");
----------------
Is there a way this could be generalized to more 64 bit machines? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631