From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id 58E403858C74; Thu, 8 Sep 2022 18:05:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 58E403858C74 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ej1-x633.google.com with SMTP id dv25so10610820ejb.12; Thu, 08 Sep 2022 11:05:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=dKiFefbv7dRDQCzyeSL6ihjv5+pP3X/3jqtZfOcFyAc=; b=Ipu/wRWuD2dLrBtcjRSVzeebg3OJ5cAVbxSkUeZgVKrmSKEPVKeG0CBmL4OFkIot8b +Md380hwGkU/YnKq94DidILd62AxDuEZSRJEx42lVthjtDAAwic0zTy4eK2Zt3WW7F4h fmMD0Pd8l920bASc8ax4Zu8JAWa5BIBClKZmSPvbHU0c4qe67/o1MtN7yUX7i3m/pMjd QMNu3A5Ffrd/w8VIy2TCh/SAG36QHpRuhiUFGEiXtahqzEZh0c/AK94GlTuD7Q244/zg VeMqsHFJV9H6uVc9JGg0uzU5HMkcPzSShmLdsp0Gk9XvaFLL7cGkLjeVzDryzLba76A7 RQxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=dKiFefbv7dRDQCzyeSL6ihjv5+pP3X/3jqtZfOcFyAc=; b=qRudtKSUBuPPMEpV/gwoqawsZ2Sc0V4DFUDQyJeC896b0Gp7N3fkVQzr84blggWM6G sLeO8Qmga9cqBOhHl9vQEYr0vFF15GRFN2b7Auc72Xa0dBqqXM/Du/tPG6oMbryFG4ZL kH7nAjE8JZJ0mAvKM4Td8R/Kdl3OO4dr85lkOzyvBgc3HJ8KEh+tCvAMmxFwzv30ppfG o5IOnGIJP6wDtGaRyn+q9qWQt/zdMvJESDe4cutMBBgmhm6JEVVPjxL710yII0Hx/AXM N+cUOm17gBeQ2s5YkqYAUATosaIkc71i3Kw7tnv1vALIlpA45hTg9UBZw19mcr8+iqc3 6KGQ== X-Gm-Message-State: ACgBeo3UuOQVJGgJXEnRc+idsE+6jZTPEgf+qkTCa4wc16fHLXMtSmZh f82SyOuXM8UiZ1MiH/OoJ+D5eCfaxwAzzUw/LtU= X-Google-Smtp-Source: AA6agR6IDdPd4sN8EvrMgJ0cTCpyA23sDlVQ0MJ4+ItTxzHmpgkRPz6+zPzNBmP5tJkzGux0xJB54d3kOZQ8kZkcUD0= X-Received: by 2002:a17:907:2e19:b0:730:acf0:4921 with SMTP id ig25-20020a1709072e1900b00730acf04921mr6531009ejc.416.1662660347024; Thu, 08 Sep 2022 11:05:47 -0700 (PDT) MIME-Version: 1.0 References: <20220905183011.43874-1-whh8b@obs.cr> <4736f71d-0579-cd42-6696-f6bf1fec0770@gmail.com> In-Reply-To: <4736f71d-0579-cd42-6696-f6bf1fec0770@gmail.com> From: Jonathan Wakely Date: Thu, 8 Sep 2022 19:05:33 +0100 Message-ID: Subject: Re: [PATCH] libstdc++: Refactor implementation of operator+ for std::string To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Will Hawkins , gcc-patches , "libstdc++" Content-Type: multipart/alternative; boundary="0000000000005dd7eb05e82e47f8" X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: --0000000000005dd7eb05e82e47f8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 8 Sep 2022, 18:51 Fran=C3=A7ois Dumont via Libstdc++, < libstdc++@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 > > --- > > 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 > > + _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. > > + __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, > _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 >=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_trait= s; > > - 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, > _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_trait= s; > > - __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 > > > --0000000000005dd7eb05e82e47f8--