From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 0CACE3858435 for ; Fri, 9 Sep 2022 20:28:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0CACE3858435 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1662755317; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TZq9QKEAPFlxJyyATsOIilZgAFr2w4SmHBvv0UmH7Yk=; b=LltKaPPXUiUJYc5axQ9X2NpFRxGUJPwiRgkkVg0Po82MUcux0e56LZeJz72FB97V0HpiO1 Kp8b+iHg5QSNuxOuzBfG1Z+lSyqHy4pnMlgiXfSZ1mhp7ht6dS9ER46OOYnc//HQHTFqID M87IWojwg7ace333nD3uT5H8Rc3huEk= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-661-bGimNIcQOqSKwWga1Wy5rQ-1; Fri, 09 Sep 2022 16:28:36 -0400 X-MC-Unique: bGimNIcQOqSKwWga1Wy5rQ-1 Received: by mail-qv1-f72.google.com with SMTP id w19-20020a0562140b3300b0049cad77df78so1990586qvj.6 for ; Fri, 09 Sep 2022 13:28:36 -0700 (PDT) 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=TZq9QKEAPFlxJyyATsOIilZgAFr2w4SmHBvv0UmH7Yk=; b=XP3eDHaFvpwKtyw3JS0e2EyajI7WaEupSBNtf3XO7pId+tNFHHYpUj8dpOy6wNFF6l dl5+RPMIjMyhuk9uhfULYfxOA97l8sp6Hwk4tra7rF1CzoK63+9+0h++UMorgpHtecd9 SyUzLOs7XSvwWLqmcB2Pj0ckVJayXHIMiJTcew4bfiQ1lWttib1GZTZZuqbIpN8C7hiI 3oDm+mwMwtYYl5rHbuZKd83MrHGBFsGTOJi/BDH/dsESW5wjHYL1Wu8OI8v3YmXB72d6 af/B6ZPXZpKqkEdO/x+DfkbieqlWSX+vGEH2MCfQhPSfDkZxUcveeC8y1bhuugWC4+kz QtYQ== X-Gm-Message-State: ACgBeo2kFSg6Gm0okNNMYOw4njbKAgoXvwFQUAhk/7fqIzgnJrj091in 1rS76wk48rkJXPralUIUAeSFqW5JM5BFPTrfgqjoQuEuAdadplrkkoyWsGMFiSRioand8wI4c2Q mJ8QAIQj7jQsT20/Rl0nRo8q3DWsilHk= X-Received: by 2002:ad4:5be2:0:b0:476:7e0d:815f with SMTP id k2-20020ad45be2000000b004767e0d815fmr13186812qvc.57.1662755316113; Fri, 09 Sep 2022 13:28:36 -0700 (PDT) X-Google-Smtp-Source: AA6agR4lbp5JleTuygOspMP13v9rAm6CNMAajvA77jY3LllSf5VmjnghSLpuheVjUj1eQoM9+fF11L7PBTdm/wgRAZg= X-Received: by 2002:ad4:5be2:0:b0:476:7e0d:815f with SMTP id k2-20020ad45be2000000b004767e0d815fmr13186796qvc.57.1662755315928; Fri, 09 Sep 2022 13:28:35 -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: Fri, 9 Sep 2022 21:28:25 +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@gcc.gnu.org, libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable 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, 8 Sept 2022 at 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 ope= rator+ > > on std::string that makes sure we always use the best allocation strate= gy. > > > > I have attempted to learn from all the feedback that I got on a previou= s > > 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 concatenat= ion > > 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/in= clude/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. As I said, it will never make a difference, so there's no technical reason to change it. I suppose data() is a little more expressive here, in that we only care about the characters, not the null terminator that c_str() implies (even though data() has the null terminator too, as it's the same pointer returned). > > > + __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 ? Because it's a one line function that just calls another function. That's an ideal candidate for being inline.