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

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

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

Good point!



================
Comment at: libcxx/CREDITS.TXT:29
 D: Visibility fixes, minor FreeBSD portability patches.
 
 N: Holger Arnold
----------------
ldionne wrote:
> @Mordante You might want to credit yourself for the part of the work you did on landing this.
Good point. I even discovered I never added myself to the credits.


================
Comment at: libcxx/src/include/ryu/d2s_intrinsics.h:49
+
+#if defined(_M_X64) && defined(_LIBCPP_COMPILER_MSVC)
+#define _LIBCPP_INTRINSIC128 1
----------------
mstorsjo wrote:
> I think you could loosen this into `#if defined(_M_X64) && defined(_MSC_VER)`; `_LIBCPP_COMPILER_MSVC` is only defined when building with the actual MSVC, not clang-cl impersonating MSVC. (And the real MSVC isn't supported for building libcxx in practice.)
> 
> Or did this codepath give errors when built with clang-cl before?
I only had an issue when the guard was `#ifdef _M_X64`. I'll apply your suggestion and let the CI be the judge.


================
Comment at: libcxx/src/include/ryu/d2s_intrinsics.h:71
+    (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__)) || \
+    (defined(_M_X64) && !defined(__MINGW32__) && !defined(_LIBCPP_COMPILER_MSVC)))
+#define _LIBCPP_INTRINSIC128 1
----------------
mstorsjo wrote:
> Isn't this condition kinda opposite to what you suggest in the comment? I.e. the comment writes (and we discussed) that mingw does support int128, but then you check `defined(_M_X64) && !defined(__MINGW32__)` - I believe this codepath isn't used here as intended.
> 
> Actually, I would suggest simplifying this whole condition into this:
> 
> ```
> #elif defined(__SIZEOF_INT128__) && ( \
>     (defined(__clang__) && !defined(_MSC_VER)) || \
>     (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__)))
> ```
> 
> Thus, if we have `__SIZEOF_INT128__` defined (this should exclude all 32 bit targets, so we don't need to check for any 64 bit architecture specifically). This check in itself is almost enough as such. But then as a extra requirement, we'd only try this if we also have specifically clang or gcc. And for clang, we'd exclude building in MSVC mode, where we might not have all the runtime functions available. (If it turns out that the code below doesn't actually generate any calls to runtime helpers, we wouldn't need the check for `_MSC_VER` at all.)
> 
> Or it could even be as simple as this:
> ```
> #elif !defined(_LIBCPP_HAS_NO_INT128) && !defined(_MSC_VER)
> ```
> 
> We already have `_LIBCPP_HAS_NO_INT128` in libcxx, and it might be good to honor it. However that define is only based on compiler support - in MSVC mode, it doesn't know that we actually don't have all the necessary runtime functions.
> 
> > We have __uint128 support in clang or gcc or MinGW 64
> 
> Those three aren't really three comparable things - when building for mingw, you'd be using either clang or gcc. Maybe "We have __uint128 support in gcc and clang, except when in MSVC mode" (or clang-cl instead of MSVC)?
> 
I've changed it to your first suggestion, that matches the original code better. I could use `_LIBCPP_HAS_NO_INT128` but that's defined in terms of `__SIZEOF_INT128__`. I prefer to keep this as much to the original code submitted by Microsoft to keep the size of the diff with their code-base as small as possible.


================
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}}
----------------
ldionne wrote:
> 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.
Good to know, I wasn't sure what the process was to make sure it's enabled in the next Apple product.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70631