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

@STL_MSFT @jorgbrown  @ulfjack

Thanks a lot for your work (and anyone else that's participated). I've left a few comments, but obviously this patch is huge. @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.

If you want to stay responsible for the patch, do you need help with running the libc++ test suite under various language versions? Right now I see several language conformance errors popping up (like using `inline` variables prior to C++17), and those are going to be easy to fix if you can just compile locally. Otherwise I can do that and point it to you, but it's not very efficient.

In D70631#1761886 <https://reviews.llvm.org/D70631#1761886>, @expnkx wrote:

> PLEASE DON'T ACCEPT THIS STUPID pull request.


Please refrain from commenting on libc++/libc++abi reviews until you can do so without being rude. Making technical points about a specification or an implementation thereof is OK, calling other people's work stupid isn't.



================
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.
----------------
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.


================
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',
----------------
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.


================
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
----------------
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?


================
Comment at: libcxx/test/std/utilities/charconv/charconv.msvc/double_scientific_precision_to_chars_test_cases_1.hpp:39
+
+#pragma once
+
----------------
We normally use regular include guards, it would be great to use that instead.


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

https://reviews.llvm.org/D70631