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


================
Comment at: libcxx/include/__charconv/ryu.h:63
+
+// https://github.com/ulfjack/ryu/tree/59661c3/ryu
+
----------------
STL_MSFT wrote:
> Mordante wrote:
> > STL_MSFT wrote:
> > > ldionne wrote:
> > > > Mordante wrote:
> > > > > ldionne wrote:
> > > > > > If this code is taken from https://github.com/ulfjack/ryu/tree/59661c3/ryu, would it be possible/make sense to have a script that automatically re-derives it? I'm just asking because it would be awesome if we could slurp in any updates to that library automatically.
> > > > > I would love to be able to automatically sync this code with upstream. I've no idea how much the uglification and libc++ specific attributes make this process feasible.
> > > > Well, actually, if we embed this implementation in the dylib exclusively, these headers could all move to `src/` and we would not even need to uglify them.
> > > Note that I extensively modified the upstream sources, not just for uglification and headerization but also to add bounds checking to every attempted buffer write. I kept a record of the changes I made (there's a git repo), but they aren't automatically applicable (rebasing to keep up with upstream commits was an entirely manual process, and I haven't performed such an update in a while; we've instead been modifying microsoft/STL directly). I was able to contribute much of my work upstream (in particular the performance tuning and some testing), but ran out of time for upstreaming the bounds checking in a form that would make sense in Ulf's C implementation.
> > Good point about moving the implementation to `src/`, that's already on my todo list for this patch. I prefer to look at syncing with Ulf's and MSVC STL's upstream after this patch has landed. When most of the implementation details are in the dylib there will be less ABI concerns.
> > 
> > @STL_MSFT The git repo you mentioned is that the public MSVC STL repo at GitHub?
> @Mordante 
> > The git repo you mentioned is that the public MSVC STL repo at GitHub?
> 
> No - the microsoft/STL repo contains the final result of my transformations to Ulf's original code, but it doesn't record how I transformed it (which happened shortly before we went open-source). https://github.com/StephanTLavavej/ryu/commits/msvc-2019.04.29 is what I was referring to. This is not intended to be an alternative to ulfjack/ryu, nor can it be compiled as-is - it simply has a sequence of commits that transforms the code into the final result for charconv. This allowed me to digest changes from upstream by rebasing. (As the date indicates, I haven't touched this in over 2 years. Later changes to charconv haven't been applied back to this repo.)
Thanks!


================
Comment at: libcxx/include/charconv:1699
+    const chars_format _Fmt) noexcept /* strengthened */ {
+    return _Floating_to_chars<_Floating_to_chars_overload::_Format_only>(
+        _First, _Last, static_cast<double>(_Value), _Fmt, 0);
----------------
ldionne wrote:
> It should be possible to move these `_Floating_to_chars` functions to the dylib as well. In fact, I think what I'd do is move the definition of `to_chars` itself to the dylib, so that it's the only thing that needs to remain ABI stable. We know that won't change, since it's part of the spec.
I agree more can be moved to the dylib. I just wanted to move a small part to see whether it works without issues, before moving everything. Especially since after moving some of the "decoration macros" need to be removed. Unfortunately it seems the move ran into some issues with `[[no_unique_address]]` with `clang-cl`. I posted a message in Discord about it last weekend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631