On Thu, Sep 8, 2022 at 2:05 PM Jonathan Wakely <jwakely....@gmail.com> wrote: > > > > On Thu, 8 Sep 2022, 18:51 François Dumont via Libstdc++, > <libstd...@gcc.gnu.org> wrote: >> >> On 05/09/22 20:30, Will Hawkins wrote: >> > Based on Jonathan's work, here is a patch for the implementation of >> > operator+ >> > on std::string that makes sure we always use the best allocation strategy. >> > >> > I have attempted to learn from all the feedback that I got on a previous >> > submission -- I hope I did the right thing. >> > >> > Passes abi and conformance testing on x86-64 trunk. >> > >> > Sincerely, >> > Will >> > >> > -- >8 -- >> > >> > Create a single function that performs one-allocation string concatenation >> > that can be used by various different version of operator+. >> > >> > libstdc++-v3/ChangeLog: >> > >> > * include/bits/basic_string.h: >> > Add common function that performs single-allocation string >> > concatenation. (__str_cat) >> > Use __str_cat to perform optimized operator+, where relevant. >> > * include/bits/basic_string.tcc:: >> > Remove single-allocation implementation of operator+. >> > >> > Signed-off-by: Will Hawkins <wh...@obs.cr> >> > --- >> > libstdc++-v3/include/bits/basic_string.h | 66 ++++++++++++++++------ >> > libstdc++-v3/include/bits/basic_string.tcc | 41 -------------- >> > 2 files changed, 49 insertions(+), 58 deletions(-) >> > >> > diff --git a/libstdc++-v3/include/bits/basic_string.h >> > b/libstdc++-v3/include/bits/basic_string.h >> > index 0df64ea98ca..4078651fadb 100644 >> > --- a/libstdc++-v3/include/bits/basic_string.h >> > +++ b/libstdc++-v3/include/bits/basic_string.h >> > @@ -3481,6 +3481,24 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 >> > _GLIBCXX_END_NAMESPACE_CXX11 >> > #endif >> > >> > + template<typename _Str> >> > + _GLIBCXX20_CONSTEXPR >> > + inline _Str >> > + __str_concat(typename _Str::value_type const* __lhs, >> > + typename _Str::size_type __lhs_len, >> > + typename _Str::value_type const* __rhs, >> > + typename _Str::size_type __rhs_len, >> > + typename _Str::allocator_type const& __a) >> > + { >> > + typedef typename _Str::allocator_type allocator_type; >> > + typedef __gnu_cxx::__alloc_traits<allocator_type> _Alloc_traits; >> > + _Str __str(_Alloc_traits::_S_select_on_copy(__a)); >> > + __str.reserve(__lhs_len + __rhs_len); >> > + __str.append(__lhs, __lhs_len); >> > + __str.append(__rhs, __rhs_len); >> > + return __str; >> > + } >> > + >> > // operator+ >> > /** >> > * @brief Concatenate two strings. >> > @@ -3490,13 +3508,14 @@ _GLIBCXX_END_NAMESPACE_CXX11 >> > */ >> > template<typename _CharT, typename _Traits, typename _Alloc> >> > _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT, _Traits, _Alloc> >> > + inline basic_string<_CharT, _Traits, _Alloc> >> > operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, >> > const basic_string<_CharT, _Traits, _Alloc>& __rhs) >> > { >> > - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); >> > - __str.append(__rhs); >> > - return __str; >> > + typedef basic_string<_CharT, _Traits, _Alloc> _Str; >> > + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(), >> > + __rhs.c_str(), __rhs.size(), >> >> You should use data() rather than c_str() here and all other operators. >> >> It is currently the same but is more accurate in your context. Maybe one >> day it will make a difference. > > > > I don't think so, that would be a major breaking change, for no benefit. I > think it's safe to assume they will always stay equivalent now.
Happy to make any changes to the patch that the group thinks are necessary! Will > > >> >> > + __lhs.get_allocator()); >> > } >> > >> > /** >> > @@ -3507,9 +3526,16 @@ _GLIBCXX_END_NAMESPACE_CXX11 >> > */ >> > template<typename _CharT, typename _Traits, typename _Alloc> >> > _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT,_Traits,_Alloc> >> > + inline basic_string<_CharT,_Traits,_Alloc> >> >> Why inlining ? >> >> I guess it is done this way to limit code bloat. >> >> > operator+(const _CharT* __lhs, >> > - const basic_string<_CharT,_Traits,_Alloc>& __rhs); >> > + const basic_string<_CharT,_Traits,_Alloc>& __rhs) >> > + { >> > + __glibcxx_requires_string(__lhs); >> > + typedef basic_string<_CharT, _Traits, _Alloc> _Str; >> > + return std::__str_concat<_Str>(__lhs, _Traits::length(__lhs), >> > + __rhs.c_str(), __rhs.size(), >> > + __rhs.get_allocator()); >> > + } >> > >> > /** >> > * @brief Concatenate character and string. >> > @@ -3519,8 +3545,14 @@ _GLIBCXX_END_NAMESPACE_CXX11 >> > */ >> > template<typename _CharT, typename _Traits, typename _Alloc> >> > _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT,_Traits,_Alloc> >> > - operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Alloc>& >> > __rhs); >> > + inline basic_string<_CharT,_Traits,_Alloc> >> > + operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Alloc>& >> > __rhs) >> > + { >> > + typedef basic_string<_CharT, _Traits, _Alloc> _Str; >> > + return std::__str_concat<_Str>(__builtin_addressof(__lhs), 1, >> > + __rhs.c_str(), __rhs.size(), >> > + __rhs.get_allocator()); >> > + } >> > >> > /** >> > * @brief Concatenate string and C string. >> > @@ -3534,11 +3566,12 @@ _GLIBCXX_END_NAMESPACE_CXX11 >> > operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, >> > const _CharT* __rhs) >> > { >> > - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); >> > - __str.append(__rhs); >> > - return __str; >> > + __glibcxx_requires_string(__rhs); >> > + typedef basic_string<_CharT, _Traits, _Alloc> _Str; >> > + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(), >> > + __rhs, _Traits::length(__rhs), >> > + __lhs.get_allocator()); >> > } >> > - >> > /** >> > * @brief Concatenate string and character. >> > * @param __lhs First string. >> > @@ -3550,11 +3583,10 @@ _GLIBCXX_END_NAMESPACE_CXX11 >> > inline basic_string<_CharT, _Traits, _Alloc> >> > operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, _CharT >> > __rhs) >> > { >> > - typedef basic_string<_CharT, _Traits, _Alloc> __string_type; >> > - typedef typename __string_type::size_type __size_type; >> > - __string_type __str(__lhs); >> > - __str.append(__size_type(1), __rhs); >> > - return __str; >> > + typedef basic_string<_CharT, _Traits, _Alloc> _Str; >> > + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(), >> > + __builtin_addressof(__rhs), 1, >> > + __lhs.get_allocator()); >> > } >> > >> > #if __cplusplus >= 201103L >> > diff --git a/libstdc++-v3/include/bits/basic_string.tcc >> > b/libstdc++-v3/include/bits/basic_string.tcc >> > index 4563c61429a..07a94d36757 100644 >> > --- a/libstdc++-v3/include/bits/basic_string.tcc >> > +++ b/libstdc++-v3/include/bits/basic_string.tcc >> > @@ -599,47 +599,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > #else >> > # define _GLIBCXX_STRING_CONSTEXPR >> > #endif >> > - >> > - template<typename _CharT, typename _Traits, typename _Alloc> >> > - _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT, _Traits, _Alloc> >> > - operator+(const _CharT* __lhs, >> > - const basic_string<_CharT, _Traits, _Alloc>& __rhs) >> > - { >> > - __glibcxx_requires_string(__lhs); >> > - typedef basic_string<_CharT, _Traits, _Alloc> __string_type; >> > - typedef typename __string_type::size_type __size_type; >> > - typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template >> > - rebind<_CharT>::other _Char_alloc_type; >> > - typedef __gnu_cxx::__alloc_traits<_Char_alloc_type> _Alloc_traits; >> > - const __size_type __len = _Traits::length(__lhs); >> > - __string_type __str(_Alloc_traits::_S_select_on_copy( >> > - __rhs.get_allocator())); >> > - __str.reserve(__len + __rhs.size()); >> > - __str.append(__lhs, __len); >> > - __str.append(__rhs); >> > - return __str; >> > - } >> > - >> > - template<typename _CharT, typename _Traits, typename _Alloc> >> > - _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT, _Traits, _Alloc> >> > - operator+(_CharT __lhs, const basic_string<_CharT, _Traits, _Alloc>& >> > __rhs) >> > - { >> > - typedef basic_string<_CharT, _Traits, _Alloc> __string_type; >> > - typedef typename __string_type::size_type __size_type; >> > - typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template >> > - rebind<_CharT>::other _Char_alloc_type; >> > - typedef __gnu_cxx::__alloc_traits<_Char_alloc_type> _Alloc_traits; >> > - __string_type __str(_Alloc_traits::_S_select_on_copy( >> > - __rhs.get_allocator())); >> > - const __size_type __len = __rhs.size(); >> > - __str.reserve(__len + 1); >> > - __str.append(__size_type(1), __lhs); >> > - __str.append(__rhs); >> > - return __str; >> > - } >> > - >> > template<typename _CharT, typename _Traits, typename _Alloc> >> > _GLIBCXX_STRING_CONSTEXPR >> > typename basic_string<_CharT, _Traits, _Alloc>::size_type >> >>