[libc++] Remove the usage of __init in operator+

[philnik]

Summary

operator+ currently calls __init. This patch removes the usage of implementation details.

[ldionne]

libcxx/include/string

4180–4189

IMO this makes a lot more sense. However, I'm going to push the reasoning further and ask this: Would this *also* be equivalent?

using _String = basic_string<_CharT, _Traits, _Allocator>;
typename _String::size_type __lhs_sz = __lhs.size();
typename _String::size_type __rhs_sz = __rhs.size();
_String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
__r.reserve(__lhs_sz + __rhs_sz);
__r.append(__lhs.data(), __lhs_sz);
__r.append(__rhs.data(), __rhs_sz);
return __r;

If so, then perhaps we don't really need the constructor after all?

[philnik]

libcxx/include/string

4180–4189

It think this is equivalent for runtime. For constant evaluation the constructor saves an allocation, but I don't think we care enough about that to add a new constructor.

[philnik]

• Address comments

[Mordante]

LGTM! Since @ldionne had some request I leave the final approval to him.

[ldionne]

We just looked at the codegen for the current code and the reserve-based version, and the reserve-based version ends up calling reserve, append and push_back separately, whereas the old version is simpler (it calls operator new and memcpy directly): https://godbolt.org/z/EWs78arre. I think this is due to the fact that the old version (with __init) can assume that we don't have any constructed state before, so it doesn't need to be as complicated. Also, push_back is exported in the dylib (which is a strange choice IMO since we ought to have a fast path). Can you run a quick benchmark to see if there's a noticeable difference? If so, we could go with this patch but also try to provide a fast path for push_back (which is probably a good idea regardless).

[philnik]

• Use constructor

[philnik]

The old constructor style was a bit slower than the __init approach. The new one is significantly faster.
Here is the benchmark:

StringOpPlus.cpp5 KBDownload

[ldionne]

libcxx/include/string

751

Let's make it explicit.

Can you also add a comment documenting what this constructor does? Something like:

// Construct a string with the given allocator and enough storage to hold `__size` characters, but
// don't initialize the characters. The contents of the string, including the null terminator, must be
// initialized separately.

753

We need to handle the case where __size > max_size() like we used to in __init.

4205

This would need to be value_type() instead of '\0'. We support non-char character types as well.

[philnik]

• Address comments
• Try to CI

[philnik]

• Try to fix CI

[philnik]

Rebased

[ldionne]

LGTM, thanks!

libcxx/include/string

 typedef basic_string<char8_t> u8string;
 #endif
 
 #ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
 typedef basic_string<char16_t> u16string;
 typedef basic_string<char32_t> u32string;
 #endif
 
+struct __uninitialized_size_tag {};
+
 template<class _CharT, class _Traits, class _Allocator>
 class
     _LIBCPP_TEMPLATE_VIS
 #ifndef _LIBCPP_HAS_NO_CHAR8_T
     _LIBCPP_PREFERRED_NAME(u8string)
 #endif
 #ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
     _LIBCPP_PREFERRED_NAME(u16string)
struct __rep
             __long  __l;
             __short __s;
             __raw   __r;
         };
     };
 
     __compressed_pair<__rep, allocator_type> __r_;
 
+    // Construct a string with the given allocator and enough storage to hold `__size` characters, but
+    // don't initialize the characters. The contents of the string, including the null terminator, must be
+    // initialized separately.
+    _LIBCPP_HIDE_FROM_ABI explicit basic_string(__uninitialized_size_tag, size_type __size, const allocator_type& __a)
+            : __r_(__default_init_tag(), __a) {
+        if (__size > max_size())
+            __throw_length_error();
+        if (__fits_in_sso(__size)) {
+            __zero();
+            __set_short_size(__size);
+        } else {
+            auto __capacity = __recommend(__size) + 1;
+            auto __allocation = __alloc_traits::allocate(__alloc(), __capacity);
+            __set_long_cap(__capacity);
+            __set_long_pointer(__allocation);
+            __set_long_size(__size);
+        }
+        std::__debug_db_insert_c(this);
+    }
+
 public:
     _LIBCPP_TEMPLATE_DATA_VIS
     static const size_type npos = -1;
 
     _LIBCPP_INLINE_VISIBILITY basic_string()
         _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value);
 
     _LIBCPP_INLINE_VISIBILITY explicit basic_string(const allocator_type& __a)

 
 // operator +
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
           const basic_string<_CharT, _Traits, _Allocator>& __rhs)
 {
     using _String = basic_string<_CharT, _Traits, _Allocator>;
-    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
-    typename _String::size_type __lhs_sz = __lhs.size();
-    typename _String::size_type __rhs_sz = __rhs.size();
-    __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
-    __r.append(__rhs.data(), __rhs_sz);
+    auto __lhs_sz = __lhs.size();
+    auto __rhs_sz = __rhs.size();
+    _String __r(__uninitialized_size_tag(),
+                __lhs_sz + __rhs_sz,
+                _String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    auto __ptr = std::__to_address(__r.__get_pointer());
+    _Traits::copy(__ptr, __lhs.data(), __lhs_sz);
+    _Traits::copy(__ptr + __lhs_sz, __rhs.data(), __rhs_sz);
+    _Traits::assign(__ptr + __lhs_sz + __rhs_sz, 1, _CharT());
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const _CharT* __lhs , const basic_string<_CharT,_Traits,_Allocator>& __rhs)
 {
     using _String = basic_string<_CharT, _Traits, _Allocator>;
-    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
-    typename _String::size_type __lhs_sz = _Traits::length(__lhs);
-    typename _String::size_type __rhs_sz = __rhs.size();
-    __r.__init(__lhs, __lhs_sz, __lhs_sz + __rhs_sz);
-    __r.append(__rhs.data(), __rhs_sz);
+    auto __lhs_sz = _Traits::length(__lhs);
+    auto __rhs_sz = __rhs.size();
+    _String __r(__uninitialized_size_tag(),
+                __lhs_sz + __rhs_sz,
+                _String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
+    auto __ptr = std::__to_address(__r.__get_pointer());
+    _Traits::copy(__ptr, __lhs, __lhs_sz);
+    _Traits::copy(__ptr + __lhs_sz, __rhs.data(), __rhs_sz);
+    _Traits::assign(__ptr + __lhs_sz + __rhs_sz, 1, _CharT());
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Allocator>& __rhs)
 {
     using _String = basic_string<_CharT, _Traits, _Allocator>;
-    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
     typename _String::size_type __rhs_sz = __rhs.size();
-    __r.__init(&__lhs, 1, 1 + __rhs_sz);
-    __r.append(__rhs.data(), __rhs_sz);
+    _String __r(__uninitialized_size_tag(),
+                __rhs_sz + 1,
+                _String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
+    auto __ptr = std::__to_address(__r.__get_pointer());
+    _Traits::assign(__ptr, 1, __lhs);
+    _Traits::copy(__ptr + 1, __rhs.data(), __rhs_sz);
+    _Traits::assign(__ptr + 1 + __rhs_sz, 1, _CharT());
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 inline
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs, const _CharT* __rhs)
 {
     using _String = basic_string<_CharT, _Traits, _Allocator>;
-    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
     typename _String::size_type __lhs_sz = __lhs.size();
     typename _String::size_type __rhs_sz = _Traits::length(__rhs);
-    __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
-    __r.append(__rhs, __rhs_sz);
+    _String __r(__uninitialized_size_tag(),
+                __lhs_sz + __rhs_sz,
+                _String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    auto __ptr = std::__to_address(__r.__get_pointer());
+    _Traits::copy(__ptr, __lhs.data(), __lhs_sz);
+    _Traits::copy(__ptr + __lhs_sz, __rhs, __rhs_sz);
+    _Traits::assign(__ptr + __lhs_sz + __rhs_sz, 1, _CharT());
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs, _CharT __rhs)
 {
     using _String = basic_string<_CharT, _Traits, _Allocator>;
-    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
     typename _String::size_type __lhs_sz = __lhs.size();
-    __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + 1);
-    __r.push_back(__rhs);
+    _String __r(__uninitialized_size_tag(),
+                __lhs_sz + 1,
+                _String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    auto __ptr = std::__to_address(__r.__get_pointer());
+    _Traits::copy(__ptr, __lhs.data(), __lhs_sz);
+    _Traits::assign(__ptr + __lhs_sz, 1, __rhs);
+    _Traits::assign(__ptr + 1 + __lhs_sz, 1, _CharT());
     return __r;
 }
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template<class _CharT, class _Traits, class _Allocator>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>