[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 marked 7 inline comments as done.
Mordante added inline comments.


================
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'};
----------------
lichray wrote:
> 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.
Since `std::to_chars` and `std::to_string` are both in the dylib this isn't an issue anymore.


================
Comment at: libcxx/src/include/ryu/d2fixed.h:53
+
+[[nodiscard]] to_chars_result __d2fixed_buffered_n(char* _First, char* const _Last, const double __d, const uint32_t __precision);
+[[nodiscard]] to_chars_result __d2exp_buffered_n(char* _First, char* const _Last, const double __d, uint32_t __precision);
----------------
Mordante wrote:
> ldionne wrote:
> > Mordante wrote:
> > > @ldionne Do you have an idea what's wrong here. I declare this function here and define it in `src/ryu/d2fixed.cpp`. Do you see anything wrong with this? Am I missing some visibility macro's?
> > > 
> > > The reason I ask is since this cause causes a ICE in Clang when building with `-DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_ENABLE_ASSERTIONS=ON`. This combination is used on our CI's bootstrap build. 
> > Is this supposed to be exported from the dylib? If so, it needs `_LIBCPP_FUNC_VIS` IIRC. Otherwise, no visibility macro should be fine (it will automatically be hidden since we build the dylib with `-fvisibility=hidden`, which you should be able to confirm by looking at the build log).
> > 
> > I think the best course of action here would be to reduce the Clang ICE, since an ICE is always a bug.
> It's supposed to be hidden in the dylib. This is part of the functions that we want to hide to allow us to change the implementation of the floating point algorithm without introducing ABI breaks. The dylib is indeed build with `-fvisibility=hidden`.
> 
> I agree and ICE is a bug and I want to report it after reducing it. For now I wanted to make sure I didn't overlook anything obvious. Thanks for your input.
> Is this supposed to be exported from the dylib? If so, it needs `_LIBCPP_FUNC_VIS` IIRC. Otherwise, no visibility macro should be fine (it will automatically be hidden since we build the dylib with `-fvisibility=hidden`, which you should be able to confirm by looking at the build log).
> 
> I think the best course of action here would be to reduce the Clang ICE, since an ICE is always a bug.

Reduced and reported as https://bugs.llvm.org/show_bug.cgi?id=52584


================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/double_scientific_precision_to_chars_test_cases_1.hpp:39
+
+#pragma once
+
----------------
STL_MSFT wrote:
> ldionne wrote:
> > We normally use regular include guards, it would be great to use that instead.
> Agreed. We could even make that change in microsoft/STL's tests if you want to reduce divergence.
@STL_MSFT If you're willing to update the headers in Microsoft's that would be great. Then I can sync them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631