[libc++] Simplify the implementation of string::{append,assign,assign_range}

[philnik777]

__string_is_trivial_iterator_v already implies that the iterator is contiguous. There is no need to specialize for input ranges specifically.

[github-actions]

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

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

__string_is_trivial_iterator_v already implies that the iterator is contiguous. There is no need to specialize for input ranges specifically.

---

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

1 Files Affected:

• (modified) libcxx/include/string (+6-22)

diff --git a/libcxx/include/string b/libcxx/include/string
index 363f27a51648c..5e10aa4671621 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1413,24 +1413,16 @@ public:
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* _LIBCPP_DIAGNOSE_NULLPTR __s);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(size_type __n, value_type __c);
 
-  template <class _InputIterator, __enable_if_t<__has_exactly_input_iterator_category<_InputIterator>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
-  append(_InputIterator __first, _InputIterator __last) {
-    const basic_string __temp(__first, __last, __alloc_);
-    append(__temp.data(), __temp.size());
-    return *this;
-  }
-
-  template <class _ForwardIterator, __enable_if_t<__has_forward_iterator_category<_ForwardIterator>::value, int> = 0>
+  template <class _InputIterator>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
-  append(_ForwardIterator __first, _ForwardIterator __last) {
+  append(_InputIterator __first, _InputIterator __last) {
     size_type __sz  = size();
     size_type __cap = capacity();
     size_type __n   = static_cast<size_type>(std::distance(__first, __last));
     if (__n == 0)
       return *this;
 
-    if (__string_is_trivial_iterator_v<_ForwardIterator> && !__addr_in_range(*__first)) {
+    if (__string_is_trivial_iterator_v<_InputIterator> && !__addr_in_range(*__first)) {
       if (__cap - __sz < __n)
         __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
       __annotate_increase(__n);
@@ -1540,17 +1532,10 @@ public:
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& assign(const value_type* _LIBCPP_DIAGNOSE_NULLPTR __s);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& assign(size_type __n, value_type __c);
 
-  template <class _InputIterator, __enable_if_t<__has_exactly_input_iterator_category<_InputIterator>::value, int> = 0>
+  template <class _InputIterator>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
   assign(_InputIterator __first, _InputIterator __last) {
-    __assign_with_sentinel(__first, __last);
-    return *this;
-  }
-
-  template <class _ForwardIterator, __enable_if_t<__has_forward_iterator_category<_ForwardIterator>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
-  assign(_ForwardIterator __first, _ForwardIterator __last) {
-    if (__string_is_trivial_iterator_v<_ForwardIterator>) {
+    if _LIBCPP_CONSTEXPR (__string_is_trivial_iterator_v<_InputIterator>) {
       size_type __n = static_cast<size_type>(std::distance(__first, __last));
       __assign_trivial(__first, __last, __n);
     } else {
@@ -1563,8 +1548,7 @@ public:
 #  if _LIBCPP_STD_VER >= 23
   template <_ContainerCompatibleRange<_CharT> _Range>
   _LIBCPP_HIDE_FROM_ABI constexpr basic_string& assign_range(_Range&& __range) {
-    if constexpr (__string_is_trivial_iterator_v<ranges::iterator_t<_Range>> &&
-                  (ranges::forward_range<_Range> || ranges::sized_range<_Range>)) {
+    if constexpr (__string_is_trivial_iterator_v<ranges::iterator_t<_Range>>) {
       size_type __n = static_cast<size_type>(ranges::distance(__range));
       __assign_trivial(ranges::begin(__range), ranges::end(__range), __n);
 

[ldionne]

I guess this could be something like

string __tmp;
__tmp.reserve(__sz + __n);
__tmp.assign(begin(), end());
__tmp.insert(__tmp.end(), __first, __last);
swap(*this, __tmp);

And then we don't have to care about __addr_in_range(*__first) anymore. That's basically the algorithm that we do in vector as well.

This can be done in a follow-up patch.

[frederick-vs-ja]

For new testing iterators, I thing it's worthy adding (T, Iter) overload. See also #160732 and #161049.

Suggested change

	template <class T>
	void operator,(const T&) = delete;
	template <class T>
	void operator,(const T&) = delete;
	template <class T>
	friend void operator,(const T&, const single_pass_iterator&) = delete;

[ldionne]

The purpose of __string_is_trivial_iterator, as far as I remember and looking at https://reviews.llvm.org/D98573, is to identify iterators that basically don't throw in their operations (e.g. operator++). I don't know whether it still makes sense to check that or whether we can do better in our implementation to accommodate such iterators, but I think that we should document its purpose in a historically accurate way.