[libc++] Complete `<charconv>` for 64-bit `long double` platforms #117125

frederick-vs-ja · 2024-11-21T07:50:21Z

Towards #99940 and #99952. Implementation of P0067R5 + P0682R1 becomes complete for these platforms.

llvmbot · 2024-11-21T07:50:54Z

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

Towards #99940 and #99952. Implementation of P0067R5 + P0682R1 becomes complete for these platforms.

Full diff: https://github.com/llvm/llvm-project/pull/117125.diff

11 Files Affected:

(modified) libcxx/docs/FeatureTestMacroTable.rst (+2-2)
(modified) libcxx/docs/ReleaseNotes/20.rst (+4)
(modified) libcxx/docs/Status/Cxx17Papers.csv (+2-2)
(modified) libcxx/include/__charconv/from_chars_floating_point.h (+12)
(modified) libcxx/include/__config (+6)
(modified) libcxx/include/version (+6-2)
(modified) libcxx/test/std/language.support/support.limits/support.limits.general/charconv.version.compile.pass.cpp (+12-12)
(modified) libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp (+12-12)
(modified) libcxx/test/std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp (+22)
(modified) libcxx/test/support/charconv_test_helpers.h (+5-1)
(modified) libcxx/utils/generate_feature_test_macro_components.py (+5-2)

diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index 3c4a13332661ee..597d0ddf3ff299 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -152,7 +152,7 @@ Status
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_string_view``                                  ``201606L``
     ---------------------------------------------------------- -----------------
-    ``__cpp_lib_to_chars``                                     *unimplemented*
+    ``__cpp_lib_to_chars``                                     ``201611L``
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_transparent_operators``                        ``201510L``
     ---------------------------------------------------------- -----------------
@@ -490,7 +490,7 @@ Status
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_text_encoding``                                *unimplemented*
     ---------------------------------------------------------- -----------------
-    ``__cpp_lib_to_chars``                                     *unimplemented*
+    ``__cpp_lib_to_chars``                                     ``202306L``
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_to_string``                                    *unimplemented*
     ---------------------------------------------------------- -----------------
diff --git a/libcxx/docs/ReleaseNotes/20.rst b/libcxx/docs/ReleaseNotes/20.rst
index 9039c6f046445b..89c803d3b836bf 100644
--- a/libcxx/docs/ReleaseNotes/20.rst
+++ b/libcxx/docs/ReleaseNotes/20.rst
@@ -38,7 +38,11 @@ What's New in Libc++ 20.0.0?
 Implemented Papers
 ------------------
 
+- P0067R5: Elementary string conversions is implemented for platforms
+  where ``long double`` has the same format as ``double`` (`Github <https://github.com/llvm/llvm-project/issues/99940>`__)
 - P0619R4: Reviewing Deprecated Facilities of C++17 for C++20 (`Github <https://github.com/llvm/llvm-project/issues/99985>`__)
+- P0682R1: Repairing elementary string conversions is implemented for platforms
+  where ``long double`` has the same format as ``double`` (`Github <https://github.com/llvm/llvm-project/issues/99952>`__)
 - P2747R2: ``constexpr`` placement new (`Github <https://github.com/llvm/llvm-project/issues/105427>`__)
 - P2609R3: Relaxing Ranges Just A Smidge (`Github <https://github.com/llvm/llvm-project/issues/105253>`__)
 - P2985R0: A type trait for detecting virtual base classes (`Github <https://github.com/llvm/llvm-project/issues/105432>`__)
diff --git a/libcxx/docs/Status/Cxx17Papers.csv b/libcxx/docs/Status/Cxx17Papers.csv
index a589207085d36f..69f6ccc1db9844 100644
--- a/libcxx/docs/Status/Cxx17Papers.csv
+++ b/libcxx/docs/Status/Cxx17Papers.csv
@@ -71,7 +71,7 @@
 "`P0394R4 <https://wg21.link/P0394R4>`__","Hotel Parallelifornia: terminate() for Parallel Algorithms Exception Handling","2016-06 (Oulu)","|Complete|","17",""
 "","","","","",""
 "`P0003R5 <https://wg21.link/P0003R5>`__","Removing Deprecated Exception Specifications from C++17","2016-11 (Issaquah)","|Complete|","5",""
-"`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 v7; for ``float`` and ``double``, ``std::to_chars`` since v14 and ``std::from_chars`` since v20. Support is complete except for ``long double``."
+"`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 v7; for ``float`` and ``double``, ``std::to_chars`` since v14 and ``std::from_chars`` since v20. Support is complete except for ``long double`` on platforms where its format is different ``double``."
 "`P0403R1 <https://wg21.link/P0403R1>`__","Literal suffixes for ``basic_string_view``\ ","2016-11 (Issaquah)","|Complete|","4",""
 "`P0414R2 <https://wg21.link/P0414R2>`__","Merging shared_ptr changes from Library Fundamentals to C++17","2016-11 (Issaquah)","|Complete|","11",""
 "`P0418R2 <https://wg21.link/P0418R2>`__","Fail or succeed: there is no atomic lattice","2016-11 (Issaquah)","","",""
@@ -109,5 +109,5 @@
 "`P0618R0 <https://wg21.link/P0618R0>`__","Deprecating <codecvt>","2017-02 (Kona)","|Complete|","15",""
 "`P0623R0 <https://wg21.link/P0623R0>`__","Final C++17 Parallel Algorithms Fixes","2017-02 (Kona)","|Nothing To Do|","",""
 "","","","","",""
-"`P0682R1 <https://wg21.link/P0682R1>`__","Repairing elementary string conversions","2017-07 (Toronto)","","",""
+"`P0682R1 <https://wg21.link/P0682R1>`__","Repairing elementary string conversions","2017-07 (Toronto)","|Partial|","","Support is complete in v20 on platforms where ``long double`` has the same format as ``double``."
 "`P0739R0 <https://wg21.link/P0739R0>`__","Some improvements to class template argument deduction integration into the standard library","2017-07 (Toronto)","|Complete|","5",""
diff --git a/libcxx/include/__charconv/from_chars_floating_point.h b/libcxx/include/__charconv/from_chars_floating_point.h
index 811e518a81db70..62152a392f51b5 100644
--- a/libcxx/include/__charconv/from_chars_floating_point.h
+++ b/libcxx/include/__charconv/from_chars_floating_point.h
@@ -64,6 +64,18 @@ from_chars(const char* __first, const char* __last, double& __value, chars_forma
   return std::__from_chars<double>(__first, __last, __value, __fmt);
 }
 
+#  if _LIBCPP_LONG_DOUBLE_IS_DOUBLE
+_LIBCPP_AVAILABILITY_FROM_CHARS_FLOATING_POINT _LIBCPP_HIDE_FROM_ABI inline from_chars_result
+from_chars(const char* __first, const char* __last, long double& __value, chars_format __fmt = chars_format::general) {
+  double __dval;
+  const auto __result = std::__from_chars<double>(__first, __last, __dval, __fmt);
+  if (__result.ec == errc{})
+    __value = __dval;
+  return __result;
+}
+// TODO: Complete the implementation for platforms where long double has a different format from double.
+#  endif
+
 #endif // _LIBCPP_STD_VER >= 17
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 9db00cd0c9fb93..8e8759a073e741 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -1231,6 +1231,12 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HAS_EXPLICIT_THIS_PARAMETER 0
 #  endif
 
+#  if defined(_MSC_VER) || __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__
+#    define _LIBCPP_LONG_DOUBLE_IS_DOUBLE 1
+#  else
+#    define _LIBCPP_LONG_DOUBLE_IS_DOUBLE 0
+#  endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP___CONFIG
diff --git a/libcxx/include/version b/libcxx/include/version
index fc57aeade9daf2..c57f391e3f5dc7 100644
--- a/libcxx/include/version
+++ b/libcxx/include/version
@@ -357,7 +357,9 @@ __cpp_lib_void_t                                        201411L <type_traits>
 # define __cpp_lib_shared_ptr_arrays                    201611L
 # define __cpp_lib_shared_ptr_weak_type                 201606L
 # define __cpp_lib_string_view                          201606L
-// # define __cpp_lib_to_chars                             201611L
+# if _LIBCPP_LONG_DOUBLE_IS_DOUBLE
+#   define __cpp_lib_to_chars                           201611L
+# endif
 # undef  __cpp_lib_transparent_operators
 # define __cpp_lib_transparent_operators                201510L
 # define __cpp_lib_type_trait_variable_templates        201510L
@@ -572,8 +574,10 @@ __cpp_lib_void_t                                        201411L <type_traits>
 # define __cpp_lib_string_view                          202403L
 // # define __cpp_lib_submdspan                            202306L
 // # define __cpp_lib_text_encoding                        202306L
+# if _LIBCPP_LONG_DOUBLE_IS_DOUBLE
 # undef  __cpp_lib_to_chars
-// # define __cpp_lib_to_chars                             202306L
+#   define __cpp_lib_to_chars                           202306L
+# endif
 // # define __cpp_lib_to_string                            202306L
 # undef  __cpp_lib_tuple_like
 // # define __cpp_lib_tuple_like                           202311L
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/charconv.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/charconv.version.compile.pass.cpp
index cc38cbacd51b73..cd83a5740c5fb5 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/charconv.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/charconv.version.compile.pass.cpp
@@ -50,16 +50,16 @@
 #   error "__cpp_lib_constexpr_charconv should not be defined before c++23"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++17"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++17"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -69,16 +69,16 @@
 #   error "__cpp_lib_constexpr_charconv should not be defined before c++23"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++20"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++20"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -91,16 +91,16 @@
 #   error "__cpp_lib_constexpr_charconv should have the value 202207L in c++23"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++23"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++23"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -113,16 +113,16 @@
 #   error "__cpp_lib_constexpr_charconv should have the value 202207L in c++26"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++26"
 #   endif
 #   if __cpp_lib_to_chars != 202306L
 #     error "__cpp_lib_to_chars should have the value 202306L in c++26"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
index b0f8b2f80067d5..215a9ecd4a83e9 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
@@ -3081,16 +3081,16 @@
 #   error "__cpp_lib_to_array should not be defined before c++20"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++17"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++17"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -4471,16 +4471,16 @@
 #   error "__cpp_lib_to_array should have the value 201907L in c++20"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++20"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++20"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -6077,16 +6077,16 @@
 #   error "__cpp_lib_to_array should have the value 201907L in c++23"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++23"
 #   endif
 #   if __cpp_lib_to_chars != 201611L
 #     error "__cpp_lib_to_chars should have the value 201611L in c++23"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
@@ -8007,16 +8007,16 @@
 #   error "__cpp_lib_to_array should have the value 201907L in c++26"
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if defined(TEST_LONG_DOUBLE_IS_DOUBLE)
 #   ifndef __cpp_lib_to_chars
 #     error "__cpp_lib_to_chars should be defined in c++26"
 #   endif
 #   if __cpp_lib_to_chars != 202306L
 #     error "__cpp_lib_to_chars should have the value 202306L in c++26"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_to_chars
-#     error "__cpp_lib_to_chars should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_to_chars should not be defined when the requirement 'defined(TEST_LONG_DOUBLE_IS_DOUBLE)' is not met!"
 #   endif
 # endif
 
diff --git a/libcxx/test/std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp b/libcxx/test/std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp
index 6faf0499c4c9bb..95b04214aa8824 100644
--- a/libcxx/test/std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp
+++ b/libcxx/test/std/utilities/charconv/charconv.from.chars/floating_point.pass.cpp
@@ -15,6 +15,9 @@
 //
 // from_chars_result from_chars(const char* first, const char* last,
 //                              double& value, chars_format fmt = chars_format::general)
+//
+// from_chars_result from_chars(const char* first, const char* last,
+//                              long double& value, chars_format fmt = chars_format::general)
 
 #include <array>
 #include <charconv>
@@ -1518,6 +1521,25 @@ struct test_hex {
 //   test/std/utilities/charconv/charconv.msvc/test.cpp
 // uses random values. This tests contains errors found by this test.
 void test_random_errors() {
+#ifdef TEST_LONG_DOUBLE_IS_DOUBLE
+  {
+    const char* s    = "4.219902180869891e-2788";
+    const char* last = s + std::strlen(s) - 1;
+
+    // last + 1 contains a digit. When that value is parsed the exponent is
+    // e-2788 which returns std::errc::result_out_of_range and the value 0.
+    // the proper exponent is e-278, which can be represented by a
+    // long double whose format is same as double.
+
+    long double value             = 0.25L;
+    std::from_chars_result result = std::from_chars(s, last, value);
+
+    assert(result.ec == std::errc{});
+    assert(result.ptr == last);
+    assert(value == 4.219902180869891e-278L);
+  }
+  // TODO: Add more precise cases when the implementation for long double is complete.
+#endif
   {
     const char* s    = "4.219902180869891e-2788";
     const char* last = s + std::strlen(s) - 1;
diff --git a/libcxx/test/support/charconv_test_helpers.h b/libcxx/test/support/charconv_test_helpers.h
index fcae09478457b6..0ae7a66f421730 100644
--- a/libcxx/test/support/charconv_test_helpers.h
+++ b/libcxx/test/support/charconv_test_helpers.h
@@ -317,7 +317,11 @@ auto all_unsigned = type_list<
     >();
 auto integrals = concat(all_signed, all_unsigned);
 
-auto all_floats = type_list< float, double >(); //TODO: Add long double
+#ifdef TEST_LONG_DOUBLE_IS_DOUBLE // TODO: Remove this condition when the implementation for long double is complete.
+auto all_floats = type_list< float, double, long double >();
+#else
+auto all_floats = type_list< float, double >();
+#endif
 
 template <template <typename> class Fn, typename... Ts>
 TEST_CONSTEXPR_CXX23 void
diff --git a/libcxx/utils/generate_feature_test_macro_components.py b/libcxx/utils/generate_feature_test_macro_components.py
index 53fd44291b216a..b66ffd9723e72b 100755
--- a/libcxx/utils/generate_feature_test_macro_components.py
+++ b/libcxx/utils/generate_feature_test_macro_components.py
@@ -1333,7 +1333,8 @@ def add_version_header(tc):
                 "c++26": 202306,  # P2497R0 Testing for success or failure of <charconv> functions
             },
             "headers": ["charconv"],
-            "unimplemented": True,
+            "test_suite_guard": "defined(TEST_LONG_DOUBLE_IS_DOUBLE)",
+            "libcxx_guard": "_LIBCPP_LONG_DOUBLE_IS_DOUBLE",
         },
         {
             "name": "__cpp_lib_to_string",
@@ -1532,7 +1533,9 @@ def produce_macros_definition_for_std(std):
             result += "# if %s\n" % tc["libcxx_guard"]
             inner_indent += 2
         if get_value_before(tc["values"], std) is not None:
-            assert "test_suite_guard" not in tc.keys()
+            # TRANSITION, __cpp_lib_to_chars has different values
+            # but needs to be guarded.
+            # assert "test_suite_guard" not in tc.keys()
             result += "# undef  %s\n" % tc["name"]
         line = "#%sdefine %s" % ((" " * inner_indent), tc["name"])
         line += " " * (indent - len(line))

philnik777

I'm not convinced this is a good idea. We're essentially adding a bunch of conditionals where we should really just implement the long double overload. Also, IMO we should just provide the symbol for long double on every platform and not just on ones where long double and double have different representations. It's not like we're saving much by providing it only sometimes.

philnik777 · 2024-11-21T13:41:30Z

libcxx/include/__config

@@ -1231,6 +1231,12 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HAS_EXPLICIT_THIS_PARAMETER 0
 #  endif

+#  if defined(_MSC_VER) || __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__


Is it guaranteed that if they have the same size they have the same representation?

We're already using such condition in the test suite. I don't think we support any platform where their sizes are equal but their representations are not.

I'm much less concerned about such a check in our test suite than in our ABI.

I do agree it would be safer if the compiler provided some #define that told us basically "double and long double have the same size and representation". I don't know that this is provided by any compiler, though. One thing we could do is explicitly list the platforms on which we know that this is the case for certain. That way we'd be more conservative (i.e. the assumption may hold for some platforms that we wouldn't list, but at least we'd never make the assumptions for platforms where that's not true).

The way to properly check that long double is the same as double is to use LDBL_MANT_DIG == 53.

ldionne

IMO this makes sense, but I agree with @philnik777 that we should use a new entry point in the dylib for long double just like we do for the other types. IMO what this patch is buying us is mostly front-loading of work we'll have to do when we can truly support long double, which will now be simplified cause we only need to plug in the missing bits.

ldionne · 2024-11-28T19:28:50Z

libcxx/test/support/charconv_test_helpers.h

@@ -317,7 +317,11 @@ auto all_unsigned = type_list<
    >();
 auto integrals = concat(all_signed, all_unsigned);

-auto all_floats = type_list< float, double >(); //TODO: Add long double
+#ifdef TEST_LONG_DOUBLE_IS_DOUBLE // TODO: Remove this condition when the implementation for long double is complete.


This must be defined in test_macros.h!

ldionne · 2024-11-28T19:33:22Z

libcxx/include/__config

@@ -1231,6 +1231,12 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HAS_EXPLICIT_THIS_PARAMETER 0
 #  endif

+#  if defined(_MSC_VER) || __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__


I do agree it would be safer if the compiler provided some #define that told us basically "double and long double have the same size and representation". I don't know that this is provided by any compiler, though. One thing we could do is explicitly list the platforms on which we know that this is the case for certain. That way we'd be more conservative (i.e. the assumption may hold for some platforms that we wouldn't list, but at least we'd never make the assumptions for platforms where that's not true).

philnik777 · 2024-11-28T19:51:07Z

IMO what this patch is buying us is mostly front-loading of work we'll have to do when we can truly support long double, which will now be simplified cause we only need to plug in the missing bits.

Does it? AFAICT we'll just have to add

template __from_chars_result<long double> __from_chars_floating_point(
    _LIBCPP_NOESCAPE const char* __first, _LIBCPP_NOESCAPE const char* __last, chars_format __fmt);

to the dylib as well as a corresponding declaration once the LLVM libc supports long doubles.

ldionne · 2024-11-28T20:44:36Z

Does it? AFAICT we'll just have to add
template __from_chars_result<long double> __from_chars_floating_point(
    _LIBCPP_NOESCAPE const char* __first, _LIBCPP_NOESCAPE const char* __last, chars_format __fmt);
to the dylib as well as a corresponding declaration once the LLVM libc supports long doubles.

After this patch, yes that's right that's all we'd need to do (I think). That's what I mean by "this buys us a bit". It doesn't buy us much, but it doesn't do nothing either.

philnik777 · 2024-11-28T20:46:05Z

Does it? AFAICT we'll just have to add
template __from_chars_result<long double> __from_chars_floating_point(
    _LIBCPP_NOESCAPE const char* __first, _LIBCPP_NOESCAPE const char* __last, chars_format __fmt);
to the dylib as well as a corresponding declaration once the LLVM libc supports long doubles.
After this patch, yes that's right that's all we'd need to do (I think). That's what I mean by "this buys us a bit". It doesn't buy us much, but it doesn't do nothing either.

But that's exactly the same with and without this patch?

ldionne · 2024-11-28T21:26:39Z

But that's exactly the same with and without this patch?

I think we're talking past each other. I think that this patch is necessary regardless (except we wouldn't need the _LIBCPP_LONG_DOUBLE_IS_DOUBLE macro), and then

template __from_chars_result<long double> __from_chars_floating_point(
    _LIBCPP_NOESCAPE const char* __first, _LIBCPP_NOESCAPE const char* __last, chars_format __fmt);

is also necessary on top of that. Either way, I think we both agree that this patch doesn't buy us much.

@michaelrj-google Is there any plan to support long double in the foreseeable future? That would simplify everything.

philnik777 · 2024-11-28T21:59:52Z

But that's exactly the same with and without this patch?

I think we're talking past each other. I think that this patch is necessary regardless (except we wouldn't need the _LIBCPP_LONG_DOUBLE_IS_DOUBLE macro), and then
template __from_chars_result<long double> __from_chars_floating_point(
    _LIBCPP_NOESCAPE const char* __first, _LIBCPP_NOESCAPE const char* __last, chars_format __fmt);
is also necessary on top of that. Either way, I think we both agree that this patch doesn't buy us much.

But the main thing this patch does is introducing the _LIBCPP_LONG_DOUBLE_IS_DOUBLE macro to forward to the double version sometimes? There isn't much except adding the macro and some FTM configuration and a bit of testing. What am I missing here?

michaelrj-google

We already have support for most types of long double: 64 bit, 80 bit, and 128 bit. We currently don't support long double as double double (only used by zOS AFAIK), but that's the only variant that I know we're missing.

michaelrj-google · 2024-12-03T18:29:00Z

libcxx/include/__config

@@ -1231,6 +1231,12 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HAS_EXPLICIT_THIS_PARAMETER 0
 #  endif

+#  if defined(_MSC_VER) || __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__


The way to properly check that long double is the same as double is to use LDBL_MANT_DIG == 53.

philnik777 · 2024-12-03T20:00:43Z

We already have support for most types of long double: 64 bit, 80 bit, and 128 bit. We currently don't support long double as double double (only used by zOS AFAIK), but that's the only variant that I know we're missing.

Why didn't we add that in the previous patch as well?

michaelrj-google · 2024-12-03T20:48:15Z

We already have support for most types of long double: 64 bit, 80 bit, and 128 bit. We currently don't support long double as double double (only used by zOS AFAIK), but that's the only variant that I know we're missing.

Why didn't we add that in the previous patch as well?

Keeping the initial patch focused, iirc. Adding the whole hand-in-hand mechanism was enough complexity that we decided to leave long double for a followup patch.

ldionne · 2024-12-09T17:36:31Z

Ah ah! So in that case, I think we should simply enable that support and this patch is probably not worth pursuing. @frederick-vs-ja do you agree?

frederick-vs-ja · 2024-12-10T01:08:38Z

Ah ah! So in that case, I think we should simply enable that support and this patch is probably not worth pursuing. @frederick-vs-ja do you agree?

Yeah. I'm closing this PR as the simpler approach seems preferred.

frederick-vs-ja requested a review from a team as a code owner November 21, 2024 07:50

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 21, 2024

[libc++] Complete <charconv> for 64-bit long double platforms

f2c0478

frederick-vs-ja force-pushed the charconv-long-double-eq-double branch from f5c3933 to f2c0478 Compare November 21, 2024 09:14

philnik777 requested changes Nov 21, 2024

View reviewed changes

ldionne reviewed Nov 28, 2024

View reviewed changes

michaelrj-google reviewed Dec 3, 2024

View reviewed changes

frederick-vs-ja closed this Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Complete `<charconv>` for 64-bit `long double` platforms #117125

[libc++] Complete `<charconv>` for 64-bit `long double` platforms #117125

frederick-vs-ja commented Nov 21, 2024

llvmbot commented Nov 21, 2024

philnik777 left a comment

philnik777 Nov 21, 2024

frederick-vs-ja Nov 21, 2024

philnik777 Nov 21, 2024

ldionne Nov 28, 2024

michaelrj-google Dec 3, 2024

ldionne left a comment

ldionne Nov 28, 2024

ldionne Nov 28, 2024

philnik777 commented Nov 28, 2024

ldionne commented Nov 28, 2024

philnik777 commented Nov 28, 2024

ldionne commented Nov 28, 2024

philnik777 commented Nov 28, 2024

michaelrj-google left a comment

michaelrj-google Dec 3, 2024

philnik777 commented Dec 3, 2024

michaelrj-google commented Dec 3, 2024

ldionne commented Dec 9, 2024

frederick-vs-ja commented Dec 10, 2024

[libc++] Complete <charconv> for 64-bit long double platforms #117125

[libc++] Complete <charconv> for 64-bit long double platforms #117125

Conversation

frederick-vs-ja commented Nov 21, 2024

llvmbot commented Nov 21, 2024

philnik777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philnik777 commented Nov 28, 2024

ldionne commented Nov 28, 2024

philnik777 commented Nov 28, 2024

ldionne commented Nov 28, 2024

philnik777 commented Nov 28, 2024

michaelrj-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philnik777 commented Dec 3, 2024

michaelrj-google commented Dec 3, 2024

ldionne commented Dec 9, 2024

frederick-vs-ja commented Dec 10, 2024

[libc++] Complete `<charconv>` for 64-bit `long double` platforms #117125

[libc++] Complete `<charconv>` for 64-bit `long double` platforms #117125