[libc++][strings] P2591R5: Concatenation of strings and string views #88389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zingam
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Hristo Hristov (H-G-Hristov) ChangesImplemented: https://wg21.link/P2591R5 Full diff: https://github.com/llvm/llvm-project/pull/88389.diff 10 Files Affected:
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 <https://wg21.link/P2845R8>`__","LWG","Formatting of ``std::filesystem::path``","Tokyo March 2024","","","|format|"
"`P0493R5 <https://wg21.link/P0493R5>`__","LWG","Atomic minimum/maximum","Tokyo March 2024","","",""
"`P2542R8 <https://wg21.link/P2542R8>`__","LWG","``views::concat``","Tokyo March 2024","","","|ranges|"
-"`P2591R5 <https://wg21.link/P2591R5>`__","LWG","Concatenation of strings and string views","Tokyo March 2024","","",""
+"`P2591R5 <https://wg21.link/P2591R5>`__","LWG","Concatenation of strings and string views","Tokyo March 2024","|Complete|","19.0",""
"`P2248R8 <https://wg21.link/P2248R8>`__","LWG","Enabling list-initialization for algorithms","Tokyo March 2024","","",""
"`P2810R4 <https://wg21.link/P2810R4>`__","LWG","``is_debugger_present`` ``is_replaceable``","Tokyo March 2024","","",""
"`P1068R11 <https://wg21.link/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<class charT, class traits, class Allocator>
basic_string<charT, traits, Allocator>
operator+(const basic_string<charT, traits, Allocator>& lhs, charT rhs); // constexpr since C++20
+template<class charT, class traits, class Allocator>
+ constexpr basic_string<charT, traits, Allocator>
+ operator+(const basic_string<charT, traits, Allocator>& lhs,
+ type_identity_t<basic_string_view<charT, traits>> rhs); // Since C++26
+template<class charT, class traits, class Allocator>
+ constexpr basic_string<charT, traits, Allocator>
+ operator+(basic_string<charT, traits, Allocator>&& lhs,
+ type_identity_t<basic_string_view<charT, traits>> rhs); // Since C++26
+template<class charT, class traits, class Allocator>
+ constexpr basic_string<charT, traits, Allocator>
+ operator+(type_identity_t<basic_string_view<charT, traits>> lhs,
+ const basic_string<charT, traits, Allocator>& rhs); // Since C++26
+template<class charT, class traits, class Allocator>
+ constexpr basic_string<charT, traits, Allocator>
+ operator+(type_identity_t<basic_string_view<charT, traits>> lhs,
+ basic_string<charT, traits, Allocator>&& rhs); // Since C++26
+
+
template<class charT, class traits, class Allocator>
bool operator==(const basic_string<charT, traits, Allocator>& lhs,
const basic_string<charT, traits, Allocator>& 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 _CharT,
class _Traits,
class _Allocator = allocator<_CharT>,
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 <class _CharT,
class _Traits,
@@ -2407,15 +2425,15 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
template <class _CharT, class _Traits, class _Allocator>
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 <class _CharT, class _Traits, class _Allocator>
+_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator>
+operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
+ type_identity_t<basic_string_view<_CharT, _Traits>> __rhs) {
+ using _String = basic_string<_CharT, _Traits, _Allocator>;
+
+ _String __r = __lhs;
+ __r.append(__rhs);
+ return __r;
+}
+
+template <class _CharT, class _Traits, class _Allocator>
+_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator>
+operator+(basic_string<_CharT, _Traits, _Allocator>&& __lhs,
+ type_identity_t<basic_string_view<_CharT, _Traits>> __rhs) {
+ __lhs.append(__rhs);
+ return std::move(__lhs);
+}
+
+template <class _CharT, class _Traits, class _Allocator>
+_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator>
+operator+(type_identity_t<basic_string_view<_CharT, _Traits>> __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 <class _CharT, class _Traits, class _Allocator>
+_LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator>
+operator+(type_identity_t<basic_string_view<_CharT, _Traits>> __lhs,
+ basic_string<_CharT, _Traits, _Allocator>&& __rhs) {
+ __rhs.insert(0, __lhs);
+ return std::move(__rhs);
+}
+
+#endif // _LIBCPP_STD_VER >= 26
+
// swap
template <class _CharT, class _Traits, class _Allocator>
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 <stdatomic.h>
__cpp_lib_string_contains 202011L <string> <string_view>
__cpp_lib_string_resize_and_overwrite 202110L <string>
__cpp_lib_string_udls 201304L <string>
-__cpp_lib_string_view 201803L <string> <string_view>
+__cpp_lib_string_view 202403L <string> <string_view>
+ 201803L // C++20
201606L // C++17
__cpp_lib_submdspan 202306L <mdspan>
__cpp_lib_syncbuf 201803L <syncstream>
@@ -530,6 +531,8 @@ __cpp_lib_within_lifetime 202306L <type_traits>
# 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 <string_view>
@@ -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>
+
+// [string.op.plus]
+//
+// template<class charT, class traits, class Allocator>
+// constexpr basic_string<charT, traits, Allocator>
+// operator+(const basic_string<charT, traits, Allocator>& lhs,
+// type_identity_t<basic_string_view<charT, traits>> rhs); // Since C++26
+// template<class charT, class traits, class Allocator>
+// constexpr basic_string<charT, traits, Allocator>
+// operator+(basic_string<charT, traits, Allocator>&& lhs,
+// type_identity_t<basic_string_view<charT, traits>> rhs); // Since C++26
+// template<class charT, class traits, class Allocator>
+// constexpr basic_string<charT, traits, Allocator>
+// operator+(type_identity_t<basic_string_view<charT, traits>> lhs,
+// const basic_string<charT, traits, Allocator>& rhs); // Since C++26
+// template<class charT, class traits, class Allocator>
+// constexpr basic_string<charT, traits, Allocator>
+// operator+(type_identity_t<basic_string_view<charT, traits>> lhs,
+// basic_string<charT, traits, Allocator>&& rhs); // Since C++26
+
+#include <cassert>
+#include <string>
+#include <utility>
+
+#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<CharT, TraitsT, AllocT>(MAKE_CSTRING(CharT, S), MKSTR_LEN(CharT, S), a)
+#define SV(S) std::basic_string_view<CharT, TraitsT>(MAKE_CSTRING(CharT, S), MKSTR_LEN(CharT, S))
+
+template <typename CharT, typename TraitsT, typename AllocT>
+constexpr void test() {
+ AllocT allocator;
+ std::basic_string<CharT, TraitsT, AllocT> st{ST("Hello", allocator)};
+ std::basic_string_view<CharT, TraitsT> sv{SV("World")};
+
+ assert(st + sv == ST("HelloWorld", allocator));
+ assert(st + sv != ST("Hello World", allocator));
+}
+
+constexpr bool test() {
+ test<char, std::char_traits<char>, min_allocator<char>>();
+ test<char, constexpr_char_traits<char>, min_allocator<char>>();
+ test<char, std::char_traits<char>, safe_allocator<char>>();
+ test<char, constexpr_char_traits<char>, safe_allocator<char>>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+ test<wchar_t, std::char_traits<wchar_t>, min_allocator<wchar_t>>();
+ test<wchar_t, constexpr_char_traits<wchar_t>, min_allocator<wchar_t>>();
+ test<wchar_t, std::char_traits<wchar_t>, safe_allocator<wchar_t>>();
+ test<wchar_t, constexpr_char_traits<wchar_t>, safe_allocator<wchar_t>>();
+#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"],
},
|
|
✅ With the latest revision this PR passed the Python code formatter. |
| operator+(basic_string<_CharT, _Traits, _Allocator>&& __lhs, | ||
| type_identity_t<basic_string_view<_CharT, _Traits>> __rhs) { | ||
| __lhs.append(__rhs); | ||
| return std::move(__lhs); |
There was a problem hiding this comment.
Not sure whether it would be better to drop std::move() here as __lhs is move-eligible here (ditto below).
There was a problem hiding this comment.
Thank you for the review. I reverted my interpretations to the original wording in the paper, so I'll keep this as is for now as I understand it this is our practice and also the older implementations uses the same style.
Zingam
changed the title
mordante
reviewed
Apr 13, 2024
There was a problem hiding this comment.
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?
(Note I read about the performance part in the paper after I already left some performance comments.)
libcxx/include/string
Outdated
Comment on lines
4032
to
4034
| basic_string<_CharT, _Traits, _Allocator> __r = __lhs; | ||
| __r.append(__rhs); | ||
| return __r; |
There was a problem hiding this comment.
This is correct, however I wonder whether this is the optimal solution. Did you consider/benchmark different approaches like
basic_string<_CharT, _Traits, _Allocator> __r;
__r.reserve(__lhs.size() + __rhs.size());
__r.append(__lhs);
__r.append(__rhs);
or using resize_and_overwrite?
There was a problem hiding this comment.
I think select_on_container_copy_construction is needed to obtain the allocator of the result.
libcxx/include/string
Outdated
Comment on lines
4049
to
4051
| basic_string<_CharT, _Traits, _Allocator> __r = __rhs; | ||
| __r.insert(0, __lhs); | ||
| return __r; |
There was a problem hiding this comment.
This also feels odd, here we copy rhs and directly afterwards move it. Did you consider alternative approaches avoiding doing extra work?
libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
Outdated
Show resolved
Hide resolved
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!
|
|
@mordante @frederick-vs-ja Thank you for the feedback.
(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. |
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
|
@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 This branch should demonstrate: constexpr evaluation hit maximum step limit; possible infinite loop? |
This comment was marked as resolved.
This comment was marked as resolved.
|
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. |
mordante
added a commit
to mordante/llvm-project
that referenced
this pull request
May 3, 2024
This was discovered by @StephanTLavavej who provided the solution they use in MSVC STL. This solution is based on that example. The same issue affects the constexpr_char_traits which was discovered in llvm#88389. This uses the same fix. Fixes: llvm#74221
Zingam
reviewed
May 20, 2024
Comment on lines
104
to
108
| // Create a `basic_string` to workaround clang bug: | ||
| // https://github.com/llvm/llvm-project/issues/92382 | ||
| // Comparison between pointers to a string literal and some other object results in constant evaluation failure. | ||
| std::basic_string<CharT, TraitsT, AllocT> st_{x, allocator}; | ||
| std::basic_string_view<CharT, TraitsT> sv{st_}; |
There was a problem hiding this comment.
For the record this is a workaround for: #92382
|
@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? |
|
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?" |
|
@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.
@mordante Is there anything else besides the benchmarks that you'd like to be addressed? |
dangelog
reviewed
Jul 12, 2024
| assert(result == expected); | ||
| assert(result.get_allocator() == allocator); | ||
| LIBCPP_ASSERT(is_string_asan_correct(sv + std::move(st))); | ||
| } |
There was a problem hiding this comment.
Would it also make sense to test string + convertible_to_string_view ?
There was a problem hiding this comment.
Sure it would! Thank you for suggesting!
H-G-Hristov
commented
Jul 14, 2024
| // TODO: Create a `basic_string` to workaround clang bug: | ||
| // https://github.com/llvm/llvm-project/issues/92382 | ||
| // Comparison between pointers to a string literal and some other object results in constant evaluation failure. | ||
| if constexpr (std::same_as<StringViewT<CharT, TraitsT>, std::basic_string_view<CharT, TraitsT>>) { |
There was a problem hiding this comment.
FYI: I decided to not enable this test for the convertible_to_string_view case due to the Clang bug. Once it is resolved, the test should be enabled/fixed.
I still cannot resolve the issue with building the benchmark:
|
This looks very odd. I wonder whether this is a regex library issue or a different configuration error. I expect the latter since |
mordante
approved these changes
Jul 16, 2024
| _LIBCPP_HIDE_FROM_ABI constexpr basic_string<_CharT, _Traits, _Allocator> | ||
| operator+(type_identity_t<basic_string_view<_CharT, _Traits>> __lhs, | ||
| const basic_string<_CharT, _Traits, _Allocator>& __rhs); | ||
|
|
There was a problem hiding this comment.
Can you forward declare all 4 new operators for consistency.
|
@H-G-Hristov Thank you for doing the excellent work on this! |
H-G-Hristov
deleted the
hgh/libcxx/P2591R5-Concatenation-of-string-and-string-views
branch
Harini0924
pushed a commit
to Harini0924/llvm-project
that referenced
this pull request
Jul 22, 2024
…lvm#88389) Implemented: https://wg21.link/P2591R5 - https://eel.is/c++draft/string.syn - https://eel.is/c++draft/string.op.plus --------- Co-authored-by: Hristo Hristov <[email protected]>
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
…lvm#88389) Implemented: https://wg21.link/P2591R5 - https://eel.is/c++draft/string.syn - https://eel.is/c++draft/string.op.plus --------- Co-authored-by: Hristo Hristov <[email protected]>
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…88389) Summary: Implemented: https://wg21.link/P2591R5 - https://eel.is/c++draft/string.syn - https://eel.is/c++draft/string.op.plus --------- Co-authored-by: Hristo Hristov <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250903
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented: https://wg21.link/P2591R5