llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.78k stars 11.9k forks source link

[libc++][strings] P2591R5: Concatenation of strings and string views #88389

Closed H-G-Hristov closed 3 months ago

H-G-Hristov commented 6 months ago

Implemented: https://wg21.link/P2591R5

llvmbot commented 6 months ago

@llvm/pr-subscribers-libcxx

Author: Hristo Hristov (H-G-Hristov)

Changes Implemented: https://wg21.link/P2591R5 --- Full diff: https://github.com/llvm/llvm-project/pull/88389.diff 10 Files Affected: - (modified) libcxx/docs/FeatureTestMacroTable.rst (+2) - (modified) libcxx/docs/ReleaseNotes/19.rst (+1) - (modified) libcxx/docs/Status/Cxx2cPapers.csv (+1-1) - (modified) libcxx/include/string (+82-22) - (modified) libcxx/include/version (+4-1) - (modified) libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp (+3-2) - (modified) libcxx/test/std/language.support/support.limits/support.limits.general/string_view.version.compile.pass.cpp (+3-2) - (modified) libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp (+3-2) - (added) libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp (+76) - (modified) libcxx/utils/generate_feature_test_macro_components.py (+1-1) ``````````diff diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst index 3197d2cd1b271c..0b8abe38e8bd77 100644 --- a/libcxx/docs/FeatureTestMacroTable.rst +++ b/libcxx/docs/FeatureTestMacroTable.rst @@ -456,6 +456,8 @@ Status ---------------------------------------------------------- ----------------- ``__cpp_lib_sstream_from_string_view`` ``202306L`` ---------------------------------------------------------- ----------------- + ``__cpp_lib_string_view`` ``202403L`` + ---------------------------------------------------------- ----------------- ``__cpp_lib_submdspan`` *unimplemented* ---------------------------------------------------------- ----------------- ``__cpp_lib_text_encoding`` *unimplemented* diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst index 81c05b9112bd26..6fbac70593cc0b 100644 --- a/libcxx/docs/ReleaseNotes/19.rst +++ b/libcxx/docs/ReleaseNotes/19.rst @@ -45,6 +45,7 @@ Implemented Papers - P2867R2 - Remove Deprecated ``strstream``\s From C++26 - P2872R3 - Remove ``wstring_convert`` From C++26 - P3142R0 - Printing Blank Lines with ``println`` (as DR against C++23) +- P2591R5 - Concatenation of strings and string views - P2302R4 - ``std::ranges::contains`` - P1659R3 - ``std::ranges::starts_with`` and ``std::ranges::ends_with`` diff --git a/libcxx/docs/Status/Cxx2cPapers.csv b/libcxx/docs/Status/Cxx2cPapers.csv index fa11da62bc080e..8793d9385b79ae 100644 --- a/libcxx/docs/Status/Cxx2cPapers.csv +++ b/libcxx/docs/Status/Cxx2cPapers.csv @@ -55,7 +55,7 @@ "`P2845R8 `__","LWG","Formatting of ``std::filesystem::path``","Tokyo March 2024","","","|format|" "`P0493R5 `__","LWG","Atomic minimum/maximum","Tokyo March 2024","","","" "`P2542R8 `__","LWG","``views::concat``","Tokyo March 2024","","","|ranges|" -"`P2591R5 `__","LWG","Concatenation of strings and string views","Tokyo March 2024","","","" +"`P2591R5 `__","LWG","Concatenation of strings and string views","Tokyo March 2024","|Complete|","19.0","" "`P2248R8 `__","LWG","Enabling list-initialization for algorithms","Tokyo March 2024","","","" "`P2810R4 `__","LWG","``is_debugger_present`` ``is_replaceable``","Tokyo March 2024","","","" "`P1068R11 `__","LWG","Vector API for random number generation","Tokyo March 2024","","","" diff --git a/libcxx/include/string b/libcxx/include/string index a456f8cb80ee35..7758a4ae42e046 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -407,6 +407,24 @@ template basic_string operator+(const basic_string& lhs, charT rhs); // constexpr since C++20 +template + constexpr basic_string + operator+(const basic_string& lhs, + type_identity_t> rhs); // Since C++26 +template + constexpr basic_string + operator+(basic_string&& lhs, + type_identity_t> rhs); // Since C++26 +template + constexpr basic_string + operator+(type_identity_t> lhs, + const basic_string& rhs); // Since C++26 +template + constexpr basic_string + operator+(type_identity_t> lhs, + basic_string&& rhs); // Since C++26 + + template bool operator==(const basic_string& lhs, const basic_string& rhs) noexcept; // constexpr since C++20 @@ -1074,8 +1092,8 @@ public: __enable_if_t<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value && !__is_same_uncvref<_Tp, basic_string>::value, int> = 0> - _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string( - const _Tp& __t, const allocator_type& __a) + _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS + _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit basic_string(const _Tp& __t, const allocator_type& __a) : __r_(__default_init_tag(), __a) { __self_view __sv = __t; __init(__sv.data(), __sv.size()); @@ -1307,8 +1325,8 @@ public: int> = 0> _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS _LIBCPP_CONSTEXPR_SINCE_CXX20 - basic_string& - append(const _Tp& __t, size_type __pos, size_type __n = npos); + basic_string& + append(const _Tp& __t, size_type __pos, size_type __n = npos); _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s, size_type __n); _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s); @@ -1997,15 +2015,15 @@ private: _LIBCPP_CONSTEXPR_SINCE_CXX20 #if _LIBCPP_ABI_VERSION >= 2 // We want to use the function in the dylib in ABIv1 - _LIBCPP_HIDE_FROM_ABI + _LIBCPP_HIDE_FROM_ABI #endif - _LIBCPP_DEPRECATED_("use __grow_by_without_replace") void __grow_by( - size_type __old_cap, - size_type __delta_cap, - size_type __old_sz, - size_type __n_copy, - size_type __n_del, - size_type __n_add = 0); + _LIBCPP_DEPRECATED_("use __grow_by_without_replace") void __grow_by( + size_type __old_cap, + size_type __delta_cap, + size_type __old_sz, + size_type __n_copy, + size_type __n_del, + size_type __n_add = 0); _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __grow_by_without_replace( size_type __old_cap, size_type __delta_cap, @@ -2171,8 +2189,8 @@ template , class = enable_if_t<__is_allocator<_Allocator>::value> > -explicit basic_string(basic_string_view<_CharT, _Traits>, const _Allocator& = _Allocator()) - -> basic_string<_CharT, _Traits, _Allocator>; +explicit basic_string(basic_string_view<_CharT, _Traits>, + const _Allocator& = _Allocator()) -> basic_string<_CharT, _Traits, _Allocator>; template ::__ template void _LIBCPP_CONSTEXPR_SINCE_CXX20 #if _LIBCPP_ABI_VERSION >= 2 // We want to use the function in the dylib in ABIv1 - _LIBCPP_HIDE_FROM_ABI +_LIBCPP_HIDE_FROM_ABI #endif - _LIBCPP_DEPRECATED_("use __grow_by_without_replace") basic_string<_CharT, _Traits, _Allocator>::__grow_by( - size_type __old_cap, - size_type __delta_cap, - size_type __old_sz, - size_type __n_copy, - size_type __n_del, - size_type __n_add) { +_LIBCPP_DEPRECATED_("use __grow_by_without_replace") basic_string<_CharT, _Traits, _Allocator>::__grow_by( + size_type __old_cap, + size_type __delta_cap, + size_type __old_sz, + size_type __n_copy, + size_type __n_del, + size_type __n_add) { size_type __ms = max_size(); if (__delta_cap > __ms - __old_cap) __throw_length_error(); @@ -4005,6 +4023,48 @@ operator+(basic_string<_CharT, _Traits, _Allocator>&& __lhs, _CharT __rhs) { #endif // _LIBCPP_CXX03_LANG +#if _LIBCPP_STD_VER >= 26 + +template +_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator> +operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs, + type_identity_t> __rhs) { + using _String = basic_string<_CharT, _Traits, _Allocator>; + + _String __r = __lhs; + __r.append(__rhs); + return __r; +} + +template +_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator> +operator+(basic_string<_CharT, _Traits, _Allocator>&& __lhs, + type_identity_t> __rhs) { + __lhs.append(__rhs); + return std::move(__lhs); +} + +template +_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator> +operator+(type_identity_t> __lhs, + const basic_string<_CharT, _Traits, _Allocator>& __rhs) { + using _String = basic_string<_CharT, _Traits, _Allocator>; + + _String __r = __rhs; + __r.insert(0, __lhs); + return __r; +} + +template +_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator> +operator+(type_identity_t> __lhs, + basic_string<_CharT, _Traits, _Allocator>&& __rhs) { + __rhs.insert(0, __lhs); + return std::move(__rhs); +} + +#endif // _LIBCPP_STD_VER >= 26 + // swap template diff --git a/libcxx/include/version b/libcxx/include/version index 0ed77345baa71d..d7e98fdb47208d 100644 --- a/libcxx/include/version +++ b/libcxx/include/version @@ -222,7 +222,8 @@ __cpp_lib_stdatomic_h 202011L __cpp_lib_string_contains 202011L __cpp_lib_string_resize_and_overwrite 202110L __cpp_lib_string_udls 201304L -__cpp_lib_string_view 201803L +__cpp_lib_string_view 202403L + 201803L // C++20 201606L // C++17 __cpp_lib_submdspan 202306L __cpp_lib_syncbuf 201803L @@ -530,6 +531,8 @@ __cpp_lib_within_lifetime 202306L # define __cpp_lib_span_at 202311L # define __cpp_lib_span_initializer_list 202311L # define __cpp_lib_sstream_from_string_view 202306L +# undef __cpp_lib_string_view +# define __cpp_lib_string_view 202403L // # define __cpp_lib_submdspan 202306L // # define __cpp_lib_text_encoding 202306L # undef __cpp_lib_to_chars diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp index 8d944a194faf42..4fb733716b99e2 100644 --- a/libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp +++ b/libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp @@ -29,6 +29,7 @@ __cpp_lib_string_udls 201304L [C++14] __cpp_lib_string_view 201606L [C++17] 201803L [C++20] + 202403L [C++26] __cpp_lib_to_string 202306L [C++23] */ @@ -492,8 +493,8 @@ # ifndef __cpp_lib_string_view # error "__cpp_lib_string_view should be defined in c++26" # endif -# if __cpp_lib_string_view != 201803L -# error "__cpp_lib_string_view should have the value 201803L in c++26" +# if __cpp_lib_string_view != 202403L +# error "__cpp_lib_string_view should have the value 202403L in c++26" # endif # if !defined(_LIBCPP_VERSION) diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/string_view.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/string_view.version.compile.pass.cpp index a86ab2adff6a93..f3c70cf9779737 100644 --- a/libcxx/test/std/language.support/support.limits/support.limits.general/string_view.version.compile.pass.cpp +++ b/libcxx/test/std/language.support/support.limits/support.limits.general/string_view.version.compile.pass.cpp @@ -23,6 +23,7 @@ __cpp_lib_string_contains 202011L [C++23] __cpp_lib_string_view 201606L [C++17] 201803L [C++20] + 202403L [C++26] */ #include @@ -252,8 +253,8 @@ # ifndef __cpp_lib_string_view # error "__cpp_lib_string_view should be defined in c++26" # endif -# if __cpp_lib_string_view != 201803L -# error "__cpp_lib_string_view should have the value 201803L in c++26" +# if __cpp_lib_string_view != 202403L +# error "__cpp_lib_string_view should have the value 202403L in c++26" # endif #endif // TEST_STD_VER > 23 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 3ec548f56cea1d..35bbfd916ac80a 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 @@ -206,6 +206,7 @@ __cpp_lib_string_udls 201304L [C++14] __cpp_lib_string_view 201606L [C++17] 201803L [C++20] + 202403L [C++26] __cpp_lib_submdspan 202306L [C++26] __cpp_lib_syncbuf 201803L [C++20] __cpp_lib_text_encoding 202306L [C++26] @@ -7686,8 +7687,8 @@ # ifndef __cpp_lib_string_view # error "__cpp_lib_string_view should be defined in c++26" # endif -# if __cpp_lib_string_view != 201803L -# error "__cpp_lib_string_view should have the value 201803L in c++26" +# if __cpp_lib_string_view != 202403L +# error "__cpp_lib_string_view should have the value 202403L in c++26" # endif # if !defined(_LIBCPP_VERSION) diff --git a/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp b/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp new file mode 100644 index 00000000000000..efa57442997e7f --- /dev/null +++ b/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp @@ -0,0 +1,76 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20, c++23 + +// + +// [string.op.plus] +// +// template +// constexpr basic_string +// operator+(const basic_string& lhs, +// type_identity_t> rhs); // Since C++26 +// template +// constexpr basic_string +// operator+(basic_string&& lhs, +// type_identity_t> rhs); // Since C++26 +// template +// constexpr basic_string +// operator+(type_identity_t> lhs, +// const basic_string& rhs); // Since C++26 +// template +// constexpr basic_string +// operator+(type_identity_t> lhs, +// basic_string&& rhs); // Since C++26 + +#include +#include +#include + +#include "asan_testing.h" +#include "constexpr_char_traits.h" +#include "make_string.h" +#include "min_allocator.h" +#include "test_macros.h" + +#define CS(S) MAKE_CSTRING(CharT, S) +#define ST(S, a) std::basic_string(MAKE_CSTRING(CharT, S), MKSTR_LEN(CharT, S), a) +#define SV(S) std::basic_string_view(MAKE_CSTRING(CharT, S), MKSTR_LEN(CharT, S)) + +template +constexpr void test() { + AllocT allocator; + std::basic_string st{ST("Hello", allocator)}; + std::basic_string_view sv{SV("World")}; + + assert(st + sv == ST("HelloWorld", allocator)); + assert(st + sv != ST("Hello World", allocator)); +} + +constexpr bool test() { + test, min_allocator>(); + test, min_allocator>(); + test, safe_allocator>(); + test, safe_allocator>(); +#ifndef TEST_HAS_NO_WIDE_CHARACTERS + test, min_allocator>(); + test, min_allocator>(); + test, safe_allocator>(); + test, safe_allocator>(); +#endif + + return true; +} + +int main(int, char**) { + test(); + static_assert(test()); + + return 0; +} diff --git a/libcxx/utils/generate_feature_test_macro_components.py b/libcxx/utils/generate_feature_test_macro_components.py index f2b8d55c0e11b0..1afdb6475b1877 100755 --- a/libcxx/utils/generate_feature_test_macro_components.py +++ b/libcxx/utils/generate_feature_test_macro_components.py @@ -1202,7 +1202,7 @@ def add_version_header(tc): "values": { "c++17": 201606, "c++20": 201803, - # "c++26": 202403, # P2591R5: Concatenation of strings and string views + "c++26": 202403, # P2591R5: Concatenation of strings and string views }, "headers": ["string", "string_view"], }, ``````````
github-actions[bot] commented 6 months ago

:white_check_mark: With the latest revision this PR passed the Python code formatter.

hawkinsw commented 6 months ago

Thanks for working on this. In general it looks correct, but I wonder whether you considered the performance of the wording in the paper.

The paper explicitly states it does not consider this at all for the wording, see https://isocpp.org/files/papers/P2591R5.html#minimizingallocations. There is a link to https://github.com/llvm/llvm-project/blob/llvmorg-14.0.0/libcxx/include/string#L4218.

The paper also mentions an implementation by @hawkinsw (main...hawkinsw:llvm-project:P2591_string_view_concatenation) which at first glance did performance optimizations.

@hawkinsw did you benchmark your implementation?

I did not, only because we were just doing it for "fun" at that point. Now that there is wider interest, I would be more than happy to do those benchmarks. I will respond as soon as I can!

(Note I read about the performance part in the paper after I already left some performance comments.)

H-G-Hristov commented 6 months ago

@mordante @frederick-vs-ja Thank you for the feedback. I have encountered two issues.

Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/__memory/compressed_pair.h:142:5: note: constexpr evaluation hit maximum step limit; possible infinite loop?
# |   142 |     return static_cast<_Base2 const&>(*this).__get();
constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /include/c++/v1/string:867:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^

(this feels like a bug) which is even odder I am not able to reproduce the same behavior on compiler explorer

mordante commented 6 months ago

@mordante @frederick-vs-ja Thank you for the feedback. I have encountered two issues.

* Adding another step of indirection to the tests causes _constexpr evaluation hit maximum step limit; possible infinite loop_. I haven't seen this before, so I refactored the tests to use one less step:
Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/__memory/compressed_pair.h:142:5: note: constexpr evaluation hit maximum step limit; possible infinite loop?
# |   142 |     return static_cast<_Base2 const&>(*this).__get();
* The second issue is that if I use the paper wording in the code base for the implementation is that I get a compiler error with `constexpr_char_traits` when I concatenate `StringView + String`, so I used the @hawkinsw implementation (Thanks!) more of a workaround rather than optimization in the current iteration (I mean I haven't done any benchmarks yet):
constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /include/c++/v1/string:867:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^

(this feels like a bug) which is even odder I am not able to reproduce the same behavior on compiler explorer

Can you create a new branch in your repository with these issues and post the branch name here? Then I want to take a look at both issues. There is a work-around for the step limit; but based on how simple the tests are I wonder whether it's not infinite recursion.

hawkinsw commented 6 months ago

@mordante @frederick-vs-ja Thank you for the feedback. I have encountered two issues.

* Adding another step of indirection to the tests causes _constexpr evaluation hit maximum step limit; possible infinite loop_. I haven't seen this before, so I refactored the tests to use one less step:
Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/__memory/compressed_pair.h:142:5: note: constexpr evaluation hit maximum step limit; possible infinite loop?
# |   142 |     return static_cast<_Base2 const&>(*this).__get();
* The second issue is that if I use the paper wording in the code base for the implementation is that I get a compiler error with `constexpr_char_traits` when I concatenate `StringView + String`, so I used the @hawkinsw implementation (Thanks!) more of a workaround rather than optimization in the current iteration (I mean I haven't done any benchmarks yet):

This is so cool. Thank you @H-G-Hristov! I am more than happy to help with benchmarking. I will be able to start work on it tonight -- sorry for the delay!

Will

constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /include/c++/v1/string:867:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^

(this feels like a bug) which is even odder I am not able to reproduce the same behavior on compiler explorer

H-G-Hristov commented 6 months ago

Can you create a new branch in your repository with these issues and post the branch name here? Then I want to take a look at both issues. There is a work-around for the step limit; but based on how simple the tests are I wonder whether it's not infinite recursion.

@mordante Thank you for helping!

I use the pre-compiled LLVM-19 packages on Ubuntu. I have created two branches one for each issue:

More details:

This branch should demonstrate the constexpr_char_traits issue: https://github.com/llvm/llvm-project/compare/main...H-G-Hristov:llvm-project:hgh/libcxx/P2591R5-Concatenation-of-string-and-string-views-ERROR-constexpr_char_traits

# .---command stderr------------
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:154:17: error: static assertion expression is not an integral constant expression
# |   154 |   static_assert(test<char>());
# |       |                 ^~~~~~~~~~~~
# | /home/hristo/Projects/llvm-project/libcxx/test/support/constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:2902:7: note: in call to 'move(&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0], &"short"[0], 5)'
# |  2902 |       traits_type::move(__p + __pos, __s, __n);
# |       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1478:12: note: in call to 'this->insert(0, &"short"[0], 5)'
# |  1478 |     return insert(__pos1, __sv.data(), __sv.size());
# |       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4048:3: note: in call to '__r.insert<std::basic_string_view<char, constexpr_char_traits<char>>, 0>(0, __lhs)'
# |  4048 |   __r.insert(0, __lhs);
# |       |   ^~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:87:85: note: in call to 'operator+<char, constexpr_char_traits<char>, std::allocator<char>>({&"short"[0], 5}, st)'
# |    87 |     std::same_as<std::basic_string<CharT, TraitsT, AllocT>> decltype(auto) result = sv + st;
# |       |                                                                                     ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:123:3: note: in call to 'test<char, constexpr_char_traits<char>, std::allocator<char>>(&"short"[0], &""[0], &"short"[0])'
# |   123 |   test<CharT, TraitsT, AllocT>(CS("short"), CS(""), CS("short"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:144:3: note: in call to 'test<char, constexpr_char_traits<char>, std::allocator<char>>()'
# |   144 |   test<CharT, constexpr_char_traits<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:154:17: note: in call to 'test<char>()'
# |   154 |   static_assert(test<char>());
# |       |                 ^~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:157:17: error: static assertion expression is not an integral constant expression
# |   157 |   static_assert(test<wchar_t>());
# |       |                 ^~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/support/constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&L"B"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:2902:7: note: in call to 'move(&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0], &L"B"[0], 1)'
# |  2902 |       traits_type::move(__p + __pos, __s, __n);
# |       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1478:12: note: in call to 'this->insert(0, &L"B"[0], 1)'
# |  1478 |     return insert(__pos1, __sv.data(), __sv.size());
# |       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4048:3: note: in call to '__r.insert<std::basic_string_view<wchar_t, constexpr_char_traits<wchar_t>>, 0>(0, __lhs)'
# |  4048 |   __r.insert(0, __lhs);
# |       |   ^~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:87:85: note: in call to 'operator+<wchar_t, constexpr_char_traits<wchar_t>, std::allocator<wchar_t>>({&L"B"[0], 1}, st)'
# |    87 |     std::same_as<std::basic_string<CharT, TraitsT, AllocT>> decltype(auto) result = sv + st;
# |       |                                                                                     ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:128:3: note: in call to 'test<wchar_t, constexpr_char_traits<wchar_t>, std::allocator<wchar_t>>(&L"B"[0], &L"D"[0], &L"BD"[0])'
# |   128 |   test<CharT, TraitsT, AllocT>(CS("B"), CS("D"), CS("BD"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home/hristo/Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:144:3: note: in call to 'test<wchar_t, constexpr_char_traits<wchar_t>, std::allocator<wchar_t>>()'
# |   144 |   test<CharT, constexpr_char_traits<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:157:17: note: in call to 'test<wchar_t>()'
# |   157 |   static_assert(test<wchar_t>());
# |       |                 ^~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:161:17: error: static assertion expression is not an integral constant expression
# |   161 |   static_assert(test<char8_t>());
# |       |                 ^~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/support/constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&u8"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:2902:7: note: in call to 'move(&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0], &u8"short"[0], 5)'
# |  2902 |       traits_type::move(__p + __pos, __s, __n);
# |       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1478:12: note: in call to 'this->insert(0, &u8"short"[0], 5)'
# |  1478 |     return insert(__pos1, __sv.data(), __sv.size());
# |       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4048:3: note: in call to '__r.insert<std::basic_string_view<char8_t, constexpr_char_traits<char8_t>>, 0>(0, __lhs)'
# |  4048 |   __r.insert(0, __lhs);
# |       |   ^~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:87:85: note: in call to 'operator+<char8_t, constexpr_char_traits<char8_t>, std::allocator<char8_t>>({&u8"short"[0], 5}, st)'
# |    87 |     std::same_as<std::basic_string<CharT, TraitsT, AllocT>> decltype(auto) result = sv + st;
# |       |                                                                                     ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:123:3: note: in call to 'test<char8_t, constexpr_char_traits<char8_t>, std::allocator<char8_t>>(&u8"short"[0], &u8""[0], &u8"short"[0])'
# |   123 |   test<CharT, TraitsT, AllocT>(CS("short"), CS(""), CS("short"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:144:3: note: in call to 'test<char8_t, constexpr_char_traits<char8_t>, std::allocator<char8_t>>()'
# |   144 |   test<CharT, constexpr_char_traits<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:161:17: note: in call to 'test<char8_t>()'
# |   161 |   static_assert(test<char8_t>());
# |       |                 ^~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:164:17: error: static assertion expression is not an integral constant expression
# |   164 |   static_assert(test<char16_t>());
# |       |                 ^~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/support/constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&u"short"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:2902:7: note: in call to 'move(&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0], &u"short"[0], 5)'
# |  2902 |       traits_type::move(__p + __pos, __s, __n);
# |       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1478:12: note: in call to 'this->insert(0, &u"short"[0], 5)'
# |  1478 |     return insert(__pos1, __sv.data(), __sv.size());
# |       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4048:3: note: in call to '__r.insert<std::basic_string_view<char16_t, constexpr_char_traits<char16_t>>, 0>(0, __lhs)'
# |  4048 |   __r.insert(0, __lhs);
# |       |   ^~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:87:85: note: in call to 'operator+<char16_t, constexpr_char_traits<char16_t>, std::allocator<char16_t>>({&u"short"[0], 5}, st)'
# |    87 |     std::same_as<std::basic_string<CharT, TraitsT, AllocT>> decltype(auto) result = sv + st;
# |       |                                                                                     ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:123:3: note: in call to 'test<char16_t, constexpr_char_traits<char16_t>, std::allocator<char16_t>>(&u"short"[0], &u""[0], &u"short"[0])'
# |   123 |   test<CharT, TraitsT, AllocT>(CS("short"), CS(""), CS("short"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:144:3: note: in call to 'test<char16_t, constexpr_char_traits<char16_t>, std::allocator<char16_t>>()'
# |   144 |   test<CharT, constexpr_char_traits<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:164:17: note: in call to 'test<char16_t>()'
# |   164 |   static_assert(test<char16_t>());
# |       |                 ^~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:166:17: error: static assertion expression is not an integral constant expression
# |   166 |   static_assert(test<char32_t>());
# |       |                 ^~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/support/constexpr_char_traits.h:105:12: note: comparison between '&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0]' and '&U"B"[0]' has unspecified value
# |   105 |     if (s1 < s2)
# |       |            ^
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:2902:7: note: in call to 'move(&__r.__r_.__compressed_pair_elem::__value_.__compressed_pair_elem::union (anonymous union at /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:868:5).__compressed_pair_elem::__s.__compressed_pair_elem::__data_[0], &U"B"[0], 1)'
# |  2902 |       traits_type::move(__p + __pos, __s, __n);
# |       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1478:12: note: in call to 'this->insert(0, &U"B"[0], 1)'
# |  1478 |     return insert(__pos1, __sv.data(), __sv.size());
# |       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4048:3: note: in call to '__r.insert<std::basic_string_view<char32_t, constexpr_char_traits<char32_t>>, 0>(0, __lhs)'
# |  4048 |   __r.insert(0, __lhs);
# |       |   ^~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:87:85: note: in call to 'operator+<char32_t, constexpr_char_traits<char32_t>, std::allocator<char32_t>>({&U"B"[0], 1}, st)'
# |    87 |     std::same_as<std::basic_string<CharT, TraitsT, AllocT>> decltype(auto) result = sv + st;
# |       |                                                                                     ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:128:3: note: in call to 'test<char32_t, constexpr_char_traits<char32_t>, std::allocator<char32_t>>(&U"B"[0], &U"D"[0], &U"BD"[0])'
# |   128 |   test<CharT, TraitsT, AllocT>(CS("B"), CS("D"), CS("BD"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:144:3: note: in call to 'test<char32_t, constexpr_char_traits<char32_t>, std::allocator<char32_t>>()'
# |   144 |   test<CharT, constexpr_char_traits<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:166:17: note: in call to 'test<char32_t>()'
# |   166 |   static_assert(test<char32_t>());
# |       |                 ^~~~~~~~~~~~~~~~
# | 5 errors generated.
# `-----------------------------
# error: command failed with exit status: 1

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  llvm-libc++-shared.cfg.in :: std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp

This branch should demonstrate: constexpr evaluation hit maximum step limit; possible infinite loop?

https://github.com/llvm/llvm-project/compare/main...H-G-Hristov:llvm-project:hgh/libcxx/P2591R5-Concatenation-of-string-and-string-views-ERROR-constexpr-max-step-limit

# .---command stderr------------
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:168:17: error: static assertion expression is not an integral constant expression
# |   168 |   static_assert(test());
# |       |                 ^~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/__memory/compressed_pair.h:142:5: note: constexpr evaluation hit maximum step limit; possible infinite loop?
# |   142 |     return static_cast<_Base2 const&>(*this).__get();
# |       |     ^
# | /home/hristo/Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1882:100: note: in call to 'this->__r_.second()'
# |  1882 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR const allocator_type& __alloc() const _NOEXCEPT { return __r_.second(); }
# |       |                                                                                                    ^~~~~~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:1661:12: note: in call to 'this->__alloc()'
# |  1661 |     return __alloc();
# |       |            ^~~~~~~~~
# | /home//Projects/llvm-project/build/default.debug.libcxx/include/c++/v1/string:4056:80: note: in call to '__lhs.get_allocator()'
# |  4056 |                 _String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
# |       |                                                                                ^~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:70:42: note: in call to 'operator+<wchar_t, std::char_traits<wchar_t>, test_allocator<wchar_t>>(st, {&L""[0], 0})'
# |    70 |     LIBCPP_ASSERT(is_string_asan_correct(st + sv));
# |       |                                          ^~~~~~~
# | /home//Projects/llvm-project/libcxx/test/support/test_macros.h:242:35: note: expanded from macro 'LIBCPP_ASSERT'
# |   242 | #define LIBCPP_ASSERT(...) assert(__VA_ARGS__)
# |       |                                   ^~~~~~~~~~~
# | /usr/include/assert.h:93:27: note: expanded from macro 'assert'
# |    93 |      (static_cast <bool> (expr)                                         \
# |       |                           ^~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:125:3: note: in call to 'test<wchar_t, std::char_traits<wchar_t>, test_allocator<wchar_t>>(&L"this is a much longer string"[0], &L""[0], &L"this is a much longer string"[0])'
# |   125 |   test<CharT, TraitsT, AllocT>(CS("this is a much longer string"), CS(""), CS("this is a much longer string"));
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:142:3: note: in call to 'test<wchar_t, std::char_traits<wchar_t>, test_allocator<wchar_t>>()'
# |   142 |   test<CharT, std::char_traits<CharT>, test_allocator<CharT>>();
# |       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home//Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:155:3: note: in call to 'test<wchar_t>()'
# |   155 |   test<wchar_t>();
# |       |   ^~~~~~~~~~~~~~~
# | /home/hristo/Projects/llvm-project/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp:168:17: note: in call to 'test()'
# |   168 |   static_assert(test());
# |       |                 ^~~~~~
# | 1 error generated.
# `-----------------------------
# error: command failed with exit status: 1

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  llvm-libc++-shared.cfg.in :: std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
frederick-vs-ja commented 6 months ago
  • The second issue is that if I use the paper wording in the code base for the implementation is that I get a compiler error with constexpr_char_traits when I concatenate StringView + String, so I used the @hawkinsw implementation (Thanks!) more of a workaround rather than optimization in the current iteration (I mean I haven't done any benchmarks yet):

The second issue seems because of that constexpr_char_traits::move compares potentially unrelated pointers s1 and s2, which can cause constant evaluation failure when they're unrelated.

https://github.com/llvm/llvm-project/blob/d6d84b5d1448e4f2e24b467a0abcf42fe9d543e9/libcxx/test/support/constexpr_char_traits.h#L100-L118

Error can be generated if we enforce constant evaluation (Godbolt link). It seems to me that we should fix constexpr_char_traits::move.

mordante commented 6 months ago

This issue looks similar to #74221. I have an untested patch for that, but then other things came in between. I'll work on finishing that patch.

github-actions[bot] commented 5 months ago

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

mordante commented 3 months ago

@Zingam The LLVM-19 branching is in two weeks. It would be great if you have time to get this review done before that time. Is there a way for me to help you getting this done?

hawkinsw commented 3 months ago

Sorry that I haven't been able to help as much on this as I would have liked. I will echo what @mordante said, "Is there anything that I can do to help?"

H-G-Hristov commented 3 months ago

@mordante @hawkinsw Thank you for asking to help! I've been rather busy and unable to do much libc++ related work. It would be great to complete this PR in time for 19.0. What we have so far:

  1. I used Will's prototype implementation with minor adjustments (Thanks @hawkinsw)
  2. I also believe addressed the comments regarding the tests. Unfortunately I had issues with building the benchmark suite on my Ubuntu 22.04 installation. The CMake configuration would fails, so I haven't been able to do the requested benchmarks and I haven't had the time to try again. It's probably an issue with my system. I'll give the benchmarks a try again.

@mordante Is there anything else besides the benchmarks that you'd like to be addressed?

H-G-Hristov commented 3 months ago

@Zingam The LLVM-19 branching is in two weeks. It would be great if you have time to get this review done before that time. Is there a way for me to help you getting this done?

I still cannot resolve the issue with building the benchmark:

$ ninja cxx-benchmarks [1/134] Performing configure step for 'google-benchmark-libcxx' ... -- Failed to find LLVM FileCheck -- Google Benchmark version: v0.0.0, normalized to 0.0.0 -- Enabling additional flags: -DINCLUDE_DIRECTORIES=/home//llvm-project/third-party/benchmark/include -- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- success -- Performing Test HAVE_STD_REGEX -- failed to compile -- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile -- Performing Test HAVE_POSIX_REGEX -- failed to compile CMake Error at CMakeLists.txt:315 (message): Failed to determine the source files for the regular expression backend

mordante commented 3 months ago

@Zingam The LLVM-19 branching is in two weeks. It would be great if you have time to get this review done before that time. Is there a way for me to help you getting this done?

I still cannot resolve the issue with building the benchmark:

$ ninja cxx-benchmarks [1/134] Performing configure step for 'google-benchmark-libcxx' ... -- Failed to find LLVM FileCheck -- Google Benchmark version: v0.0.0, normalized to 0.0.0 -- Enabling additional flags: -DINCLUDE_DIRECTORIES=/home//llvm-project/third-party/benchmark/include -- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- success -- Performing Test HAVE_STD_REGEX -- failed to compile -- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile -- Performing Test HAVE_POSIX_REGEX -- failed to compile

This looks very odd. I wonder whether this is a regex library issue or a different configuration error. I expect the latter since <regex> is not available too.

hawkinsw commented 3 months ago

@H-G-Hristov Thank you for doing the excellent work on this!