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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D70631#2577688 <https://reviews.llvm.org/D70631#2577688>, @Mordante wrote:

> @ldionne should we enabling floating point support by a CMake configuration option which is ON by default. The tables aren't small and it could add bloat for embedded devices.

Hmm, for now I'd make them available everywhere, and in the future, I'd add a knob if that is deemed useful. I believe that most embedded implementations will dead strip everything away if they don't use it anyways, so that might never be an issue.

Once this is shipped, we'll only be missing `from_chars`, right? We should be able to use https://github.com/fastfloat/fast_float to do that. We can ask if Daniel Lemire would relicense his code.



================
Comment at: libcxx/CREDITS.TXT:171
+
+N: Zhihao Yuan
+E: lichray at gmail.com
----------------
Let's not make these NFC changes in this patch.


================
Comment at: libcxx/include/__charconv/ryu.h:63
+
+// https://github.com/ulfjack/ryu/tree/59661c3/ryu
+
----------------
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.


================
Comment at: libcxx/include/charconv:661
 
+// Floating-point implementation starts here.
+// Unlike the other parts of charconv this is only available in C++17 and
----------------
@Mordante Could we move that to its own header? I'd also move the integral version to the same header, just to have `from_chars` all in the same spot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631