From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x135.google.com (mail-lf1-x135.google.com [IPv6:2a00:1450:4864:20::135]) by sourceware.org (Postfix) with ESMTPS id F2A3238582BF for ; Fri, 9 Sep 2022 13:53:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F2A3238582BF Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=obs.cr Authentication-Results: sourceware.org; spf=none smtp.mailfrom=obs.cr Received: by mail-lf1-x135.google.com with SMTP id f9so2076894lfr.3 for ; Fri, 09 Sep 2022 06:53:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=obs-cr.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date; bh=kZEZ/EVAv4maAR+3E512dHha26Y4EjVpFPHhDBvHhb8=; b=vTgiW/X36KQhS0PD0D42L3H+1v+0nzo7+/XbRs5LGjA3iVTQLITmLiY9hz/XtfH+Wg Bheuz3f90vvBjr0Y9KbLvv/AHPCdTeOneUia7q3N7oByLp4IBn6YVfnbXAmHXszwzwb+ /arNbVMHUFmOUBYaa/4CjW+HOm7A+r7LR33CZj4KnOgMS0WAcgsSjqZWMD/nz4UueBaT /GmTkWaihrVVWlATSBVUvChTk39MBNAJ95/dF4FReg1jzz5Hi5YQvRla7l1WkRTa2Fln WIiMI8/5ZkfSQ0zI0T2bOaJgi6w5fPqCMWhdXeU/ntdfpMG1RUldBBugAxrppc+8KiBk B09Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date; bh=kZEZ/EVAv4maAR+3E512dHha26Y4EjVpFPHhDBvHhb8=; b=wpiVNIOKWtHjYrNKTIRGU84VHtPIqurMfhGPFxzCv1TSvuSE1fMCaPvoEE1Rtlx+e+ zycQBuDtFLcBprdnqHbYCqELcSRLcFhjpVT/adWF3+BySq2vkeg/qUAoVF7+hZxsNrCH d/I03XcEMev+Ox6QVdIyEDI4FMw6VTewhozCmx03aeF4brVK/JwgDcjkAWqGQZsoe2kX Q5/dCuzwmhc/fdRSVw38Fla0bw6bmSss7XH+AqueMgth5cq3OP0ugufHZwLYKAb4hMrR D5nr0GtForaFPnS4dnnLBeFpNA99uPgEsXHepn/KcnRip5nqbKUo8NZlC18lhDYeL7RE SpZw== X-Gm-Message-State: ACgBeo01vZ8hbd4hDUpaJTYVyTtSoVjQ5dIBtma4x6kuWCNFNb/GktSg e2wTIljh99OxBUd7gvX42052EQdEeAlV0nm2s7Yqpg== X-Google-Smtp-Source: AA6agR6iO1crC1pBo/WLt4Xi55cKC3T0IkZJ7WnL5ErU6j+wy/0Q+UE2GjoOsxAHypmVP6kuKS0qvPH5FP2giO0pg8Q= X-Received: by 2002:a05:6512:1696:b0:492:d11b:4819 with SMTP id bu22-20020a056512169600b00492d11b4819mr4673878lfb.354.1662731610389; Fri, 09 Sep 2022 06:53:30 -0700 (PDT) MIME-Version: 1.0 References: <20220905183011.43874-1-whh8b@obs.cr> <4736f71d-0579-cd42-6696-f6bf1fec0770@gmail.com> In-Reply-To: From: Will Hawkins Date: Fri, 9 Sep 2022 09:53:19 -0400 Message-ID: Subject: Re: [PATCH] libstdc++: Refactor implementation of operator+ for std::string To: Jonathan Wakely Cc: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= , gcc-patches , "libstdc++" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Sep 8, 2022 at 2:05 PM Jonathan Wakely wrot= e: > > > > On Thu, 8 Sep 2022, 18:51 Fran=C3=A7ois Dumont via Libstdc++, wrote: >> >> On 05/09/22 20:30, Will Hawkins wrote: >> > Based on Jonathan's work, here is a patch for the implementation of op= erator+ >> > on std::string that makes sure we always use the best allocation strat= egy. >> > >> > I have attempted to learn from all the feedback that I got on a previo= us >> > 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 concatena= tion >> > 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 >> > --- >> > 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/i= nclude/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 >> > + _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 _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 >> > _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 >> > _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 >> > _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, _C= harT __rhs) >> > { >> > - typedef basic_string<_CharT, _Traits, _Alloc> __string_type; >> > - typedef typename __string_type::size_type __size_t= ype; >> > - __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 >=3D 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 >> > - _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_trai= ts; >> > - const __size_type __len =3D _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 >> > - _GLIBCXX20_CONSTEXPR >> > - basic_string<_CharT, _Traits, _Alloc> >> > - operator+(_CharT __lhs, const basic_string<_CharT, _Traits, _Allo= c>& __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_trai= ts; >> > - __string_type __str(_Alloc_traits::_S_select_on_copy( >> > - __rhs.get_allocator())); >> > - const __size_type __len =3D __rhs.size(); >> > - __str.reserve(__len + 1); >> > - __str.append(__size_type(1), __lhs); >> > - __str.append(__rhs); >> > - return __str; >> > - } >> > - >> > template >> > _GLIBCXX_STRING_CONSTEXPR >> > typename basic_string<_CharT, _Traits, _Alloc>::size_type >> >>