[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 added inline comments.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631