[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 an inline comment as done.
Mordante added a comment.

In D70631#3188838 <https://reviews.llvm.org/D70631#3188838>, @ldionne wrote:

> Let's not forget to restore Debug information in the bootstrapping build.

I already did that after it was clear the bug needed to be resolved before this patch could land.
https://reviews.llvm.org/rGade336dee4762572bbd03085d96ce72bdbbb123b



================
Comment at: libcxx/src/include/to_chars_floating_point.h:119
+
+    const _Uint_type _Uint_value    = _VSTD::bit_cast<_Uint_type>(_Value);
+    const _Uint_type _Ieee_mantissa = _Uint_value & _Traits::_Denormal_mantissa_mask;
----------------
dblaikie wrote:
> Mordante wrote:
> > dblaikie wrote:
> > > STL_MSFT wrote:
> > > > floam wrote:
> > > > > dblaikie wrote:
> > > > > > STL_MSFT wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > This bit_cast means the to_chars functionality (intended to be available in C++17 or above) fails when used in a C++17 context, since bit_cast is only available in C++20 onwards - at least in an internal Google build this seems to be the case (possible there's something esoteric there).
> > > > > > > > > 
> > > > > > > > > Does that seem plausible/a problem that needs fixing upstream here?
> > > > > > > > Looking back at some of the review discussion, it looks like this was deemed OK because bit_cast would be available in the dylib even when compiling in an earlier language mode? But the declaration of the function is still gated on C++20 or above here: https://github.com/llvm/llvm-project/blob/572d1ecccc473ba4ddb46dd04759dc2e336f0e1c/libcxx/include/__bit/bit_cast.h#L22 so this usage fails to compile before ever worrying about whether it links correctly.
> > > > > > > Good question about upstream - it's unaffected as we used an internal helper `_Bit_cast` precisely so that we could have access to it in C++17 mode. This is https://github.com/microsoft/STL/blob/3c2fd04d441d46ec9d914d9cbb621a3bac96c3a5/stl/inc/charconv#L2081 and https://github.com/microsoft/STL/blob/3c2fd04d441d46ec9d914d9cbb621a3bac96c3a5/stl/inc/xutility#L62-L74 .
> > > > > > The code that was committed doesn't appear to use that `_Bit_cast` internal helper.
> > > > > > The helper and its use seemed to have been removed in this iteration of the proposed patch: https://reviews.llvm.org/D70631?vs=380156&id=380203#toc
> > > > > > The final commited code ( https://reviews.llvm.org/rGa8025e06fc0f2fe1bbee9e1a6f15c336bfbdcb05 ) have no mention of `_Bit_cast` and use `_VSTD::bit_cast` in several places, including this one (line 119 here that I'm commenting on).
> > > > > > 
> > > > > > Am I misunderstanding something? 
> > > > > @dblaikie They are saying that is how Microsoft solved that quandary in //their product//.
> > > > I think you're completely correct - I was merely clarifying that there is no issue that needs to be fixed upstream here, it was introduced during the later changes here.
> > > Ah, sorry, I meant "upstream" as in "LLVM" here (as contrasted to "Downstream" being "inside google" - ie: something we need to fix on our end internally).
> > > 
> > > Sounds like we're on the same page here. Sorry for the confusion.
> > This header is part of the dylib and not part of a libc++ header shipped to customers.
> > Since the dylib is always build with C++20 it's safe to use `bit_cast` here.
> > 
> > The public `std::to_chars` for floating-point is available in C++17 and newer.
> > There are lit tests ensuring the function works correctly in C++17, C++20, and C++2b.
> > 
> Ah, sure, thanks. Yeah, looks like we might be building even the library with std=gnu++17 internally & probably need to fix that. Will look into it on our side.
Thanks. I'm surprised this works since the CMake file should force C++20. I expected the `format.cpp` to require C++20, but it seems that is guarded. (That code was added while we didn't require C++20 yet, but these guards should be removed.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631