-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libcxx][libc] Hand in Hand PoC with from_chars #91651
[libcxx][libc] Hand in Hand PoC with from_chars #91651
Conversation
Thanks for working on this! Let's see what CI has to say about its prospects, and then kick off a design conversation on Discourse? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is quite promising! I left some comments but I think we can figure out a way to make this work.
libcxx/test/std/utilities/charconv/charconv.from.chars/float.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/charconv/charconv.from.chars/float.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/src/CMakeLists.txt
Outdated
@@ -197,7 +198,7 @@ split_list(LIBCXX_LINK_FLAGS) | |||
# Build the shared library. | |||
if (LIBCXX_ENABLE_SHARED) | |||
add_library(cxx_shared SHARED ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS}) | |||
target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) | |||
target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/../../libc) #TODO: Do this properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do something like this instead:
#
# runtimes/cmake/Modules/FindLibcUtils.cmake
#
# This file sets up an interface target called e.g. libc::shared-utilities that we can link against from any of the runtimes
# to get the functionality implemented in it. Concretely, we'd use it basically from libcxx/src/CMakeLists.txt as:
include(FindLibcUtils)
target_link_libraries(cxx_shared PRIVATE libc::shared-utilities) # This probably/hopefully only adds a `-I` flag.
# Possible implementation of this file:
add_library(libc::shared-utilities INTERFACE)
target_include_directories(libc::shared-utilities INTERFACE <path-to-shared-utilities>)
target_compile_features(libc::shared-utilities INTERFACE cxx_std_11) # for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a file that does this, it's not the cleanest solution but it's the best I've got for the moment.
Thanks for the review @ldionne. I've just spoken with @michaelrj-google and I'll work on the libc++ part of this patch. |
3267f34
to
ae9736e
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this with me, the code you've added looks good so far
83ed161
to
f2072fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still some work to be done on this patch, but I really like where this is going. I think the difficult part (the boundary between libc++ and libc) is very clean with this approach so I am quite happy with the patch.
|
||
const char* src = __ptr; // rename to match the libc code copied for this section. | ||
ptrdiff_t length = __last - src; | ||
_LIBCPP_ASSERT_INTERNAL(length > 0, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a nicer error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an error message, though it may still need improvement.
@@ -317,6 +317,8 @@ auto all_unsigned = type_list< | |||
>(); | |||
auto integrals = concat(all_signed, all_unsigned); | |||
|
|||
auto all_floats = type_list< float, double >(); //TODO: Add long double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should refactor this to use test/support/type_algorithms.h
instead. Doesn't have to be in this patch.
@llvm/pr-subscribers-libc @llvm/pr-subscribers-libcxx Author: Michael Jones (michaelrj-google) ChangesThis patch aims to demonstrate the utility of sharing code between libc Patch is 78.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91651.diff 24 Files Affected:
diff --git a/libc/shared/str_to_float.h b/libc/shared/str_to_float.h
new file mode 100644
index 00000000000000..da70db11f6b82b
--- /dev/null
+++ b/libc/shared/str_to_float.h
@@ -0,0 +1,29 @@
+//===-- String to float conversion utils ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SHARED_STR_TO_FLOAT_H
+#define LLVM_LIBC_SHARED_STR_TO_FLOAT_H
+
+#include "src/__support/str_to_float.h"
+
+namespace LIBC_NAMESPACE::shared {
+
+// WARNING: This is a proof of concept. In future the interface point for libcxx
+// won't be using libc internal classes.
+
+template <class T>
+inline internal::FloatConvertReturn<T> decimal_exp_to_float(
+ internal::ExpandedFloat<T> init_num, bool truncated,
+ internal::RoundDirection round, const char *__restrict num_start,
+ const size_t num_len = cpp::numeric_limits<size_t>::max()) {
+ return internal::decimal_exp_to_float(init_num, truncated, round, num_start,
+ num_len);
+}
+} // namespace LIBC_NAMESPACE::shared
+
+#endif // LLVM_LIBC_SHARED_STR_TO_FLOAT_H
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 32579272858a8e..43f9e5071f3b02 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -235,6 +235,7 @@ set(files
__bit/rotate.h
__bit_reference
__charconv/chars_format.h
+ __charconv/from_chars_floating_point.h
__charconv/from_chars_integral.h
__charconv/from_chars_result.h
__charconv/tables.h
diff --git a/libcxx/include/__charconv/from_chars_floating_point.h b/libcxx/include/__charconv/from_chars_floating_point.h
new file mode 100644
index 00000000000000..33ecf7012cfb6a
--- /dev/null
+++ b/libcxx/include/__charconv/from_chars_floating_point.h
@@ -0,0 +1,56 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
+#define _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
+
+#include <__assert>
+#include <__charconv/chars_format.h>
+#include <__charconv/from_chars_result.h>
+#include <__charconv/traits.h>
+#include <__config>
+#include <__system_error/errc.h>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_floating_point.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 17
+
+_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_EXPORTED_FROM_ABI from_chars_result
+__from_chars_floating_point(const char* __first, const char* __last, float& __value, chars_format __fmt);
+
+_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_EXPORTED_FROM_ABI from_chars_result
+__from_chars_floating_point(const char* __first, const char* __last, double& __value, chars_format __fmt);
+
+_LIBCPP_HIDE_FROM_ABI inline from_chars_result
+from_chars(const char* __first, const char* __last, float& __value, chars_format __fmt = chars_format::general) {
+ return std::__from_chars_floating_point(__first, __last, __value, __fmt);
+}
+
+_LIBCPP_HIDE_FROM_ABI inline from_chars_result
+from_chars(const char* __first, const char* __last, double& __value, chars_format __fmt = chars_format::general) {
+ return std::__from_chars_floating_point(__first, __last, __value, __fmt);
+}
+
+#endif // _LIBCPP_STD_VER >= 17
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___CHARCONV_FROM_CHARS_FLOATING_POINT_H
diff --git a/libcxx/include/__configuration/availability.h b/libcxx/include/__configuration/availability.h
index ab483a07c9c137..edd9df87ebca99 100644
--- a/libcxx/include/__configuration/availability.h
+++ b/libcxx/include/__configuration/availability.h
@@ -87,6 +87,9 @@
// in all versions of the library are available.
#if defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
+# define _LIBCPP_INTRODUCED_IN_LLVM_20 1
+# define _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE /* nothing */
+
# define _LIBCPP_INTRODUCED_IN_LLVM_19 1
# define _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE /* nothing */
@@ -132,6 +135,11 @@
// clang-format off
+// LLVM 20
+// TODO: Fill this in
+# define _LIBCPP_INTRODUCED_IN_LLVM_20 0
+# define _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE __attribute__((unavailable))
+
// LLVM 19
// TODO: Fill this in
# define _LIBCPP_INTRODUCED_IN_LLVM_19 0
@@ -375,6 +383,11 @@
#define _LIBCPP_AVAILABILITY_HAS_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19
#define _LIBCPP_AVAILABILITY_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE
+// This controls the availability of floating-point std::from_chars functions.
+// These overloads were added later than the integer overloads.
+#define _LIBCPP_AVAILABILITY_HAS_FROM_CHARS_FLOATING_POINT _LIBCPP_INTRODUCED_IN_LLVM_20
+#define _LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_INTRODUCED_IN_LLVM_20_ATTRIBUTE
+
// Define availability attributes that depend on _LIBCPP_HAS_NO_EXCEPTIONS.
// Those are defined in terms of the availability attributes above, and
// should not be vendor-specific.
diff --git a/libcxx/include/charconv b/libcxx/include/charconv
index a2e270e9316dc7..dfef8194a2a67b 100644
--- a/libcxx/include/charconv
+++ b/libcxx/include/charconv
@@ -65,6 +65,12 @@ namespace std {
constexpr from_chars_result from_chars(const char* first, const char* last,
see below& value, int base = 10); // constexpr since C++23
+ constexpr from_chars_result from_chars(const char* first, const char* last,
+ float& value, chars_format fmt);
+
+ constexpr from_chars_result from_chars(const char* first, const char* last,
+ double& value, chars_format fmt);
+
} // namespace std
*/
@@ -73,6 +79,7 @@ namespace std {
#if _LIBCPP_STD_VER >= 17
# include <__charconv/chars_format.h>
+# include <__charconv/from_chars_floating_point.h>
# include <__charconv/from_chars_integral.h>
# include <__charconv/from_chars_result.h>
# include <__charconv/tables.h>
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 13d0dce34d97e3..6c0ee59e2cc121 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -1075,18 +1075,19 @@ module std_private_bit_invert_if [system] { header "__bit/invert_if.h" }
module std_private_bit_popcount [system] { header "__bit/popcount.h" }
module std_private_bit_rotate [system] { header "__bit/rotate.h" }
-module std_private_charconv_chars_format [system] { header "__charconv/chars_format.h" }
-module std_private_charconv_from_chars_integral [system] { header "__charconv/from_chars_integral.h" }
-module std_private_charconv_from_chars_result [system] { header "__charconv/from_chars_result.h" }
-module std_private_charconv_tables [system] { header "__charconv/tables.h" }
-module std_private_charconv_to_chars [system] { header "__charconv/to_chars.h" }
-module std_private_charconv_to_chars_base_10 [system] { header "__charconv/to_chars_base_10.h" }
-module std_private_charconv_to_chars_floating_point [system] { header "__charconv/to_chars_floating_point.h" }
-module std_private_charconv_to_chars_integral [system] {
+module std_private_charconv_chars_format [system] { header "__charconv/chars_format.h" }
+module std_private_charconv_from_chars_integral [system] { header "__charconv/from_chars_integral.h" }
+module std_private_charconv_from_chars_result [system] { header "__charconv/from_chars_result.h" }
+module std_private_charconv_tables [system] { header "__charconv/tables.h" }
+module std_private_charconv_to_chars [system] { header "__charconv/to_chars.h" }
+module std_private_charconv_to_chars_base_10 [system] { header "__charconv/to_chars_base_10.h" }
+module std_private_charconv_to_chars_floating_point [system] { header "__charconv/to_chars_floating_point.h" }
+module std_private_charconv_from_chars_floating_point [system] { header "__charconv/from_chars_floating_point.h" }
+module std_private_charconv_to_chars_integral [system] {
header "__charconv/to_chars_integral.h"
export std_private_charconv_traits
}
-module std_private_charconv_to_chars_result [system] {
+module std_private_charconv_to_chars_result [system] {
header "__charconv/to_chars_result.h"
export *
}
diff --git a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 917388f86811fe..7e227263ae10ae 100644
--- a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1587,6 +1587,8 @@
{'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIxNS_22__cxx_atomic_base_implIxEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 033d9f9987fa80..e0aca153ca0a2a 100644
--- a/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -572,6 +572,8 @@
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
index 332d8abeb03e3a..d3c3012c443b0a 100644
--- a/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -572,6 +572,8 @@
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIlNS_22__cxx_atomic_base_implIlEEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
+{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
{'import_export': 'EXP', 'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'storage_mapping_class': 'DS', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index defe235a283c21..a9dd2a5851f7f5 100644
--- a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1587,6 +1587,8 @@
{'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIxNS_22__cxx_atomic_base_implIxEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
{'is_defined': True, 'name': '__ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
index 6b77cda1e2866d..55c69232375b07 100644
--- a/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-linux-android21.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1220,6 +1220,8 @@
{'is_defined': True, 'name': '_ZNSt6__ndk123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt6__ndk123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt6__ndk125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt6__ndk131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt6__ndk132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt6__ndk134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
index 3458b333dd6a9b..356440b6cfd152 100644
--- a/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1235,6 +1235,8 @@
{'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIlNS_22__cxx_atomic_base_implIlEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
index bdf90ba25c7fd9..0fc3be345fa8d3 100644
--- a/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1233,6 +1233,8 @@
{'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKNS_17__cxx_atomic_implIiNS_22__cxx_atomic_base_implIiEEEE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__123__libcpp_atomic_monitorEPVKv', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__125notify_all_at_thread_exitERNS_18condition_variableENS_11unique_lockINS_5mutexEEE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__131__arrive_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseEh', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__132__destroy_barrier_algorithm_baseEPNS_24__barrier_algorithm_baseE', 'type': 'FUNC'}
{'is_defined': True, 'name': '_ZNSt3__134__construct_barrier_algorithm_baseERl', 'type': 'FUNC'}
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index fe9d2666fa4caa..6183f42252d82b 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -31,6 +31,7 @@ set(LIBCXX_SOURCES
include/ryu/f2s.h
include/ryu/ryu.h
include/to_chars_floating_point.h
+ include/from_chars_floating_point.h
legacy_pointer_safety.cpp
memory.cpp
memory_resource.cpp
@@ -176,11 +177,13 @@ endif()
split_list(LIBCXX_COMPILE_FLAGS)
split_list(LIBCXX_LINK_FLAGS)
+include(FindLibcUtils)
+
# Build the shared library.
if (LIBCXX_ENABLE_SHARED)
add_library(cxx_shared SHARED ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
target_include_directories(cxx_shared PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
- target_link_libraries(cxx_shared PUBLIC cxx-headers libcxx-...
[truncated]
|
@@ -121,6 +121,7 @@ inline constexpr FloatFromCharsTestCase float_from_chars_test_cases[] = { | |||
"2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222" | |||
"2222222222222222222e-45", | |||
chars_format::scientific, 1006, errc{}, 0x0.000004p-126f}, | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we should not edit this file unless strictly required. This file is copied from the MSVC STL implementation. Avoiding unrelated changes makes it easier to keep the files in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've expanded the libc/shared
interface a bit to include the other pieces that are being used in libc++. Now everything on the libc++ side should be coming from the shared
namespace.
if (__offset + 1 < __n && // an exponent always needs at least one digit. | ||
std::tolower(__first[__offset]) == 'p') { | ||
++__offset; // assumes a valid exponent. | ||
LIBC_NAMESPACE::shared::StrToNumResult<int32_t> __e = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
LIBC_NAMESPACE::shared::StrToNumResult<int32_t> __e = | |
auto __e = |
int64_t temp_exponent = static_cast<int64_t>(exponent) + static_cast<int64_t>(add_to_exponent); | ||
|
||
// If the result is in the valid range, then we use it. The valid range is | ||
// also within the int32 range, so this prevents overflow issues. | ||
if (temp_exponent > LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT) { | ||
exponent = LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT; | ||
} else if (temp_exponent < -LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT) { | ||
exponent = -LIBC_NAMESPACE::shared::FPBits<_Fp>::MAX_BIASED_EXPONENT; | ||
} else { | ||
exponent = static_cast<int32_t>(temp_exponent); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use __merge_exponents
libcxx/docs/Status/Cxx17.rst
Outdated
@@ -40,7 +40,7 @@ Paper Status | |||
|
|||
.. note:: | |||
|
|||
.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. | |||
.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment would benefit a rewrite, to note what is actually still open. E.g.
.. [#note-P0067] P0067: ``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0. | |
.. [#note-P0067] P0067: for integer types, ``std::(to|from)_chars`` has been available since version 7.0; for ``float`` and ``double``, ``std::to_chars`` since version 14.0 and ``std::from_chars`` since version 20.1. Support is complete except for ``long double``, which currently uses the implementation for ``double``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment before, but forgot to publish it :-/
to_chars
has float
, double
, and long double
, where long double
is double
from_chars
only has float
and double
support, and no long double
support at all.
(Adding proper long double
support is on my todo list and using LLVM-libc that will be easier, however a lot of work will be to write proper tests for long double
. So I feel your suggestion is not correct.
Feel free to propose different wording if you have a better suggestion.
@@ -0,0 +1,1557 @@ | |||
//===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this test to floating_point.pass.cpp
libcxx/test/std/utilities/charconv/charconv.from.chars/float.pass.cpp
Outdated
Show resolved
Hide resolved
fe23413
to
fbe296e
Compare
libcxx/docs/Status/Cxx2cIssues.csv
Outdated
@@ -78,4 +78,5 @@ | |||
"","","","","","" | |||
"`LWG3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Adopted Yet","|Complete|","16.0","" | |||
"`LWG4139 <https://wg21.link/LWG4139>`__","§[time.zone.leap] recursive constraint in <=>","Not Adopted Yet","|Complete|","20.0","" | |||
"`LWG3456 <https://wg21.link/LWG3343>`__","Pattern used by std::from_chars is underspecified (option B)",,"Not Yet Adopted","|Complete|","20.0","" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the wrong LWG issue number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a copy-paste error; the number is correct, the link is not updated.
libcxx/lib/abi/CHANGELOG.TXT
Outdated
Symbol added: _ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RdNS_12chars_formatE | ||
Symbol added: _ZNSt6__ndk127__from_chars_floating_pointEPKcS1_RfNS_12chars_formatE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These symbols seem a bit wrong: it looks like they are coming from the Android CI jobs, is that possible? Android uses a special namespace instead of __1
, so it would be better to take the name generated from another Linux job since that represents better the name of the symbols being added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the libc++ changes are done I want to make a small libc change (adding comments to the functions being shared marking them as needing special handling) but other than that this looks good to me
libcxx/test/std/utilities/charconv/charconv.from.chars/floatint_point.pass.cpp
Outdated
Show resolved
Hide resolved
@@ -71,7 +71,7 @@ | |||
"`P0394R4 <https://wg21.link/P0394R4>`__","Hotel Parallelifornia: terminate() for Parallel Algorithms Exception Handling","2016-06 (Oulu)","|Complete|","17.0","" | |||
"","","","","","" | |||
"`P0003R5 <https://wg21.link/P0003R5>`__","Removing Deprecated Exception Specifications from C++17","2016-11 (Issaquah)","|Complete|","5.0","" | |||
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``." | |||
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reattaching my suggestion (clarify what's done resp. still open) after the table format changed to include notes.
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","``std::(to|from)_chars`` for integrals has been available since version 7.0. ``std::to_chars`` for ``float`` and ``double`` since version 14.0 ``std::to_chars`` for ``long double`` uses the implementation for ``double``. ``std::from_chars`` for ``float`` and ``double`` since version 20.0." | |
"`P0067R5 <https://wg21.link/P0067R5>`__","Elementary string conversions, revision 5","2016-11 (Issaquah)","|Partial|","","For integer types, ``std::(to|from)_chars`` has been available since version 7.0; for ``float`` and ``double``, ``std::to_chars`` since version 14.0 and ``std::from_chars`` since version 20.1. Support is complete except for ``long double``, which currently uses the implementation for ``double``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typed a comment before, but it seems I forgot to publish it :-/
The suggestion is not correct
14.0 has from_chars
, where long double
uses the double
implementation
20.0 has to_chars
and no long double
support at all.
I'm open to suggestions to improve the wording, feel free to prove and updated wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for the clarification - just to double-check though:
20.0 has
to_chars
and nolong double
support at all.
This PR is adding from_chars
, right (e.g. TEST_HAS_FROM_CHARS_FLOATING_POINT
)?
6f84901
to
01d5f45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments about the ABI boundary. I'm not overly attached to those, but I do think that avoiding the external template instantiation declaration and the explicit template instantiation in the .cpp
file results in simpler code for the reader -- it's not always obvious how that stuff interacts with attributes.
@h-vetinari 's comment on the status page should also be addressed.
Thanks a lot to everyone involved in this patch! It's exciting to see this kind of collaboration between different subprojects.
Everything LGTM from the libc side. I will wait to merge until @mordante is done with his edits and approves. Once that's done I will try to merge either this friday morning or monday morning. |
01d5f45
to
71b488a
Compare
template <class _Fp> | ||
_LIBCPP_EXPORTED_FROM_ABI [[gnu::pure]] __from_chars_result<_Fp> __from_chars_floating_point( | ||
[[clang::noescape]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template <class _Fp> | |
_LIBCPP_EXPORTED_FROM_ABI [[gnu::pure]] __from_chars_result<_Fp> __from_chars_floating_point( | |
[[clang::noescape]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt); | |
template <class _Fp> | |
_LIBCPP_EXPORTED_FROM_ABI [[__gnu__::__pure__]] __from_chars_result<_Fp> __from_chars_floating_point( | |
[[_Clang::__noescape__]] const char* __first, [[clang::noescape]] const char* __last, chars_format __fmt); |
Also, [[clang::noescape]]
should be behind a macro, since GCC doesn't support it.
@ldionne What's blocking re-enabling clang tidy? This would have been caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed using macros with Louis. He'll work on a patch to document how we want to use exported dylib functions; that patch should introduce that macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I removed the pure, some compilers seem not to like it. I'll investigate this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d79aabb
to
88ea875
Compare
88ea875
to
63884e3
Compare
Implements std::from_chars for float and double. The implementation uses LLVM-libc to do the real parsing. Since this is the first time libc++ uses LLVM-libc there is a bit of additional infrastructure code. The patch is based on the [RFC] Project Hand In Hand (LLVM-libc/libc++ code sharing) https://discourse.llvm.org/t/rfc-project-hand-in-hand-llvm-libc-libc-code-sharing/77701
63884e3
to
967de3d
Compare
The force push was only to update the commit message. CI was green, so this should be safe to merge. Massive thanks to @ldionne and @mordante for pushing this along from the libc++ side, I really appreciate the work you did to make this project a success. This truly wouldn't have been possible without you. Thanks as well to everyone else who contributed to this patch to make it the best it can be. I truly hope this is the first step of many. 🤝 |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7370 Here is the relevant piece of the build log for the reference
|
This buildbot is flakey, and the failure seems to be unrelated. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/4525 Here is the relevant piece of the build log for the reference
|
Turns out for double double LDBL_MANT_DIG == 106. This patch fixes the constant. Should fix the ppc buildbot. Previously: llvm#113235 llvm#113237 llvm#91651
Implements std::from_chars for float and double. The implementation uses LLVM-libc to do the real parsing. Since this is the first time libc++ uses LLVM-libc there is a bit of additional infrastructure code. The patch is based on the [RFC] Project Hand In Hand (LLVM-libc/libc++ code sharing) https://discourse.llvm.org/t/rfc-project-hand-in-hand-llvm-libc-libc-code-sharing/77701
Turns out for double double LDBL_MANT_DIG == 106. This patch fixes the constant. Should fix the ppc buildbot. Previously: llvm#113235 llvm#113237 llvm#91651
It's currently broken by #91651. Stop buildling libcxx src files until that is sorted out.
Identifiers `clang` and `noescape` are not reserved by the C++ standard, so perhaps we need to use the equivalent reserved forms. Also changes the occurrences of that attribute to a macro, following the convention for `[[_Clang::__lifetimebound__]]`. Addresses #91651 (comment).
Implements std::from_chars for float and double.
The implementation uses LLVM-libc to do the real parsing. Since this is the first time libc++
uses LLVM-libc there is a bit of additional infrastructure code. The patch is based on the
[RFC] Project Hand In Hand (LLVM-libc/libc++ code sharing)
https://discourse.llvm.org/t/rfc-project-hand-in-hand-llvm-libc-libc-code-sharing/77701