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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with comments, assuming CI passes. Also, please add a release note.

I want to thank everybody who got involved in shipping this, I'm glad we finally finished this up!

I know there's been some progress on algorithms to implement this in the recent years -- it should always be possible to swap the implementation under the hood since everything is implemented in the dylib, and the ABI boundary is not going to change.



================
Comment at: libcxx/CREDITS.TXT:29
 D: Visibility fixes, minor FreeBSD portability patches.
 
 N: Holger Arnold
----------------
@Mordante You might want to credit yourself for the part of the work you did on landing this.


================
Comment at: libcxx/include/charconv:611
+
+_LIBCPP_AVAILABILITY_TO_CHARS_FLOATING_POINT _LIBCPP_FUNC_VIS to_chars_result to_chars(char* _First, char* _Last,
+                                                                                       float _Value);
----------------
Since those are libc++'s own declarations, we should use libc++ style and use `__first` instead of `_First`. Similarly, let's put `_LIBCPP_AVAILABILITY_TO_CHARS_FLOATING_POINT _LIBCPP_FUNC_VIS` on its own line.

Finally, I think we can remove the bit of the license here as long as we retain the one in the source file?


================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:49
+
+  lib/abi/x86_64-unknown-linux-gnu
+  --------------------------------
----------------



================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/test.pass.cpp:11-13
+// to_chars requires functions in the dylib that were introduced in Mac OS 10.15.
+//
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
----------------
This is technically inaccurate since it hasn't landed yet period, let's change it to my suggestion.

I'll fix it up once it lands in one of our OSes.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/test.pass.cpp:21
+
+// XFAIL: LIBCXX-AIX-FIXME
+
----------------
Do you know why it's failing on AIX? Just curious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631