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

[Stephan T. Lavavej via Phabricator via libcxx-commits]

STL_MSFT marked 4 inline comments as done.
STL_MSFT added a comment.

> I've left a few comments, but obviously this patch is huge.

Yeah (and I'm regretting adding the 5 MB of tests to this patch, since I can barely type into Phabricator; I suspect it's making the web interface very slow). It'll be bigger if you want from_chars()! :-)

> @STL_MSFT How do you want to proceed with this? We can either back-and-forth on this review until we're happy and we can ship it (my preference),
>  or if you prefer someone from libc++ can take over the patch from here. My preference is clearly the former, but I want to avoid annoying you in case you don't have time for this.

I humbly request the latter (take over the patch) as I'm currently time-limited due to getting the microsoft/STL repo up and running for community contributions. As I'm not familiar with working in the libc++ environment, iterating with me in the loop will be extremely time-consuming for all involved. My hope is that, given that Jorg has gotten the code working on at least one libc++ platform, you can adapt the tests, and use them to verify that further changes for other platforms don't disrupt the code, which should make iterating on the code much faster.

I am, however, available to answer questions, review diffs to the code, or change microsoft/STL in ways that will make your life easier.



================
Comment at: libcxx/include/xcharconv_ryu.h:93
+// Returns __e == 0 ? 1 : ceil(log_2(5^__e)).
+_LIBCPP_NODISCARD_AFTER_CXX17 inline int32_t __pow5bits(const int32_t __e) {
+  // This approximation works up to the point that the multiplication overflows at __e = 3529.
----------------
ldionne wrote:
> As terrible as it seems, we would technically need to sprinkle `_LIBCPP_INLINE_VISIBILITY` on top of everything that we don't want to expose in the ABI of libc++. This will give those symbols hidden visibility and internal linkage, basically, allowing us to modify the implementation of these functions 
> 
> @STL_MSFT Actually, how do you guys handle ABI stability in your STL nowadays? Do you avoid changing the preconditions/postconditions of even implementation-detail functions? I think that is what libstdc++ does.
Yeah, we're careful about precondition/postcondition changes. If we need to do that to implementation-detail functions, we ensure that their mangled names change (often by numbering them with 2, 3, etc. and marking the old names as "zombies" in a special test).

I'm not sure how much you want to avoid mechanical changes like these visibility macros, in addition to things like our differing assert macros, std qualification macros, etc. If aligning with microsoft/STL sources is very important, we could decide on a common layer of macros, so that we could keep the diffs to a minimum. AFAIK, no STLs have done this before (sharing significant code) so we're in somewhat new territory. The alternative is to just make the mechanical changes in libcxx, and if any interesting optimizations appear in upstream Ryu or microsoft/STL, port them by hand.


================
Comment at: libcxx/include/xcharconv_ryu_tables.h:56
+// generation by copying pairs of digits into the final output.
+inline constexpr char __DIGIT_TABLE[200] = {
+  '0','0','0','1','0','2','0','3','0','4','0','5','0','6','0','7','0','8','0','9',
----------------
ldionne wrote:
> We need to make this code (and all other code in headers too) conditional on C++17. libc++ needs to work in C++14 and older language standards too, and
> 1. we don't want to provide `charconv` prior to C++17, and
> 2. we can't use `inline` variables before C++17, etc.
> 
> Right now, when I compile with your patch applied in C++14 mode, I get a bunch of warnings/errors about inline variables being used but not supported.
Makes sense - this is all guarded for C++17 mode in microsoft/STL, but I didn't notice the lack of guards here. If I understand the issue correctly, it's pre-existing in libc++ charconv (which immediately begins providing machinery instead of checking the standard mode), but it becomes a problem due to these inline variables.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/double_fixed_precision_to_chars_test_cases_1.hpp:1
+// Copyright (c) Microsoft Corporation.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
----------------
ldionne wrote:
> Any reason why this is in `charconv/charconv.msvc` instead of just `charconv/`? Is it because these tests are not adapted to libc++'s style?
Correct - it's an exact copy of our tests, with no attempt made to adapt them to libc++'s style yet, or to compile them etc. The intention is for them to be properly adapted and moved, instead of committed like this.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/double_scientific_precision_to_chars_test_cases_1.hpp:39
+
+#pragma once
+
----------------
ldionne wrote:
> We normally use regular include guards, it would be great to use that instead.
Agreed. We could even make that change in microsoft/STL's tests if you want to reduce divergence.


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

https://reviews.llvm.org/D70631