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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante added inline comments.


================
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;
----------------
STL_MSFT wrote:
> zoecarver wrote:
> > Why not `std::min`?
> In MSVC's STL, `std::min` is defined in `<algorithm>`, a heavyweight header that isn't otherwise needed by `<charconv>`. This appears to be the case for libcxx as well.
> 
> If you want, I can include `<algorithm>` and use `std::min`, although I recommend against doing so due to the throughput cost, if `<algorithm>` isn't already being dragged in.
> 
> In the general precision codepath, I use `std::lower_bound()` and `std::find_if()`, as in MSVC's STL they're defined in a central internal header. In libcxx, they appear to be defined in `<algorithm>`, so I'm not sure how this compiled for Jorg.
We recently moved `std::min`, to `<__algorithm/min.h>` so now use that header.


================
Comment at: libcxx/include/charconv:733
+
+    if constexpr (_IsSame<_Floating, float>::value) {
+        _Adjusted_mantissa = _Ieee_mantissa << 1; // align to hexit boundary (23 isn't divisible by 4)
----------------
ivafanas wrote:
> ldionne wrote:
> > ivafanas wrote:
> > > STL_MSFT wrote:
> > > > ivafanas wrote:
> > > > > As I remember `if constexpr` is available since C++17:
> > > > > https://en.cppreference.com/w/cpp/language/if
> > > > > 
> > > > > And `charconv` header was made usable with C++11 in libc++:
> > > > > https://reviews.llvm.org/D59598
> > > > > 
> > > > > May be it is a good idea to save header compatibility with C++11.
> > > > MSVC and Clang actually support `if constexpr` in earlier language versions (I am not sure about C++11 since MSVC supports only C++14), with a warning that can be suppressed.
> > > > 
> > > > I don't understand why someone would want to use an old language mode with a new library mode, but this shouldn't be a blocking issue for that.
> > > Hi,
> > > 
> > > > MSVC and Clang actually support if constexpr in earlier language versions
> > > 
> > > [[ http://libcxx.llvm.org/docs/ | Official documentation says ]] that GCC 5.0+ is supported but @EricWF noted [[ https://reviews.llvm.org/D63296#1542570 | here ]] about GCC 5.1+. According to cppreference `if constexpr` is supported since GCC 7+:
> > > https://en.cppreference.com/w/cpp/compiler_support
> > > 
> > > If things haven't changed too much there might be GCC 5.* or GCC 6.* with C++11 but without `if constexpr`.
> > > 
> > > > I don't understand why someone would want to use an old language mode with a new library mode
> > > 
> > > There is an explanation why libc++ needs `charconv` backport for C++11:
> > > https://reviews.llvm.org/D59598
> > > 
> > > > this shouldn't be a blocking issue for that
> > > 
> > > I'm not an expert in libc++ and do not have strong opinion.
> > > Suppose that libc++ developers should have deep knowledge on that.
> > @ivafanas 
> > 
> > I don't think it's reasonable or useful to backport this huge patch to C++11. We'll just disable the feature pre C++17.
> Understood.
> Thank you!
I see `if constexpr` used in the code so I assume this has been addressed.


================
Comment at: libcxx/include/charconv:965
+
+        return {_First + _Len, errc{}};
+    }
----------------
lichray wrote:
> It was reported some buggy gcc versions only accept `errc()`.  Worth a fix?
Since we now require at least GCC-11 I don't think this change is useful


================
Comment at: libcxx/include/xcharconv_ryu.h:173
+    (defined(__clang__) && !defined(_WIN32)) || \
+    (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__)))
+#define _LIBCPP_INTRINSIC128 1
----------------
@STL_MSFT Do you recall why you used this `#elif` instead of testing for `_LIBCPP_HAS_NO_INT128`, which basically is `!defined(__SIZEOF_INT128__)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631