From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) by sourceware.org (Postfix) with ESMTPS id 2505A385840C; Thu, 8 Sep 2022 17:50:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2505A385840C 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-ed1-x536.google.com with SMTP id a70so9412143edf.10; Thu, 08 Sep 2022 10:50:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date; bh=0dGTdruBun/SxtA3Cgw5TVocA8jjdR+WzzoeqrtYt00=; b=QbkBfjb+9FdddPlp3th8dTKGKNYVByp/gbSRydv6NCFPBMBwA0MWJu5n8C3vuuPJCZ fpoCHPU4GH3quHHKE8L8auvulp+WZ1cH3NbNd9Dl5uNxPrz9hAdDcQmrayHfzo+a2ofn BBZ7iVZVfUWkldF86w+h54TQrPtmTYS7XgZZ0yAVDZhanS91lbjo5yIbVz9pPGVEtN8W NiSw3wByQqTsTPMBTRms0vtKlGlXysCfRA+ybXcSeVA98B2cjccQ/B7C+opAuKS7lWLV IZ0KFgZw9ZiEgmnJZ62sN2I1tIr5rW/O+c3ZMvz5IWxpCU4m9MfpwT7AMg4Rm0SVTTv4 vr7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=0dGTdruBun/SxtA3Cgw5TVocA8jjdR+WzzoeqrtYt00=; b=tX19XvkrxkpigjFX7itTQwgsLnj0s8YyGXxtR7mBj1LJtPMQPQPIeoOleDok+yuMUV JUy9tVXAAFg9gE0Ps2NlgvgDt1URU1L4wcFFW+TH7H7WAMxOsjAWu6ILsX9uktbqp5ih oyEdGLecIu0jlxpU8fxq6+OH6JARGn9hrgrXFZjkJLmEl/6atYZG8iyTXSVvZs7CN0hU g/xNZuLvf13SF/VDqqyy4nNMM7J9TI6iTNx7BYYIDRzY8/bqAJK8G8IQ3+7y/c9EKnEU gLGiWbZw+h5tP1WRWyzlzvzAK1CNp+smG1RGTYlO6P0GtDUyMY1YThzIsww8mEKNqndM ZDBQ== X-Gm-Message-State: ACgBeo184UmS/llZMJfN97j5ffKiPuib26RWT3DSiE90jHnIo+DMnSoG gjS+5MN2gHCgTB1iIkp3nF1BLamNOXY= X-Google-Smtp-Source: AA6agR5jgpV14V/JjWqCS5VkUXLKbvQPYCogk1/jQF86ARpfJ9+wm3N+ZE8SzA1E1/jkrrvpvrvwzg== X-Received: by 2002:a05:6402:51ce:b0:43e:74bc:dce with SMTP id r14-20020a05640251ce00b0043e74bc0dcemr8476645edd.225.1662659442716; Thu, 08 Sep 2022 10:50:42 -0700 (PDT) Received: from [10.48.2.98] ([109.190.253.15]) by smtp.googlemail.com with ESMTPSA id o7-20020a056402444700b0044f188488f6sm3168243edb.51.2022.09.08.10.50.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 08 Sep 2022 10:50:42 -0700 (PDT) Message-ID: <4736f71d-0579-cd42-6696-f6bf1fec0770@gmail.com> Date: Thu, 8 Sep 2022 19:50:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] libstdc++: Refactor implementation of operator+ for std::string Content-Language: fr To: Will Hawkins , gcc-patches@gcc.gnu.org Cc: libstdc++@gcc.gnu.org References: <20220905183011.43874-1-whh8b@obs.cr> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= In-Reply-To: <20220905183011.43874-1-whh8b@obs.cr> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,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: 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. > + __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 >= 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_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 > - _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 > _GLIBCXX_STRING_CONSTEXPR > typename basic_string<_CharT, _Traits, _Alloc>::size_type