From patchwork Wed Nov 17 17:36:33 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 47841 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 36A0B385AC35 for ; Wed, 17 Nov 2021 17:37:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 36A0B385AC35 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1637170659; bh=s1vlaAFRjUH+8WoF20QRg0GWbgjAPW0J7RuwHNk9Oio=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=dOOLTa9vDZ8p5TFEO3QMJjMDqtFBCiF48GLx40Ee36o18/gMS8njPbFvxZcSPeTfG ijNl7fn91X9ydrIOQUFQiq7Kc4tlI4dLcbKKVLgsbS5Fjpok0NdqAyA93e8j0ThVNm MumTmdMBNjtOiymOZv6jA44PUPAkyz/oteLDoLdM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1ED973858409 for ; Wed, 17 Nov 2021 17:36:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1ED973858409 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-324-ykqqdSTMN9uqCzCxhPtNdQ-1; Wed, 17 Nov 2021 12:36:35 -0500 X-MC-Unique: ykqqdSTMN9uqCzCxhPtNdQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CE36518D6A2C; Wed, 17 Nov 2021 17:36:34 +0000 (UTC) Received: from localhost (unknown [10.33.36.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id 54D9A5F4E0; Wed, 17 Nov 2021 17:36:34 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Simplify std::string constructors Date: Wed, 17 Nov 2021 17:36:33 +0000 Message-Id: <20211117173633.4178788-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-14.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, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Tested powerpc64le-linux, pushed to trunk. Several std::basic_string constructors dispatch to one of the two-argument overloads of _M_construct, which then dispatches again to _M_construct_aux to detect whether the arguments are iterators or not. That then dispatches to one of _M_construct(size_type, char_type) or _M_construct(Iter, Iter, iterator_traits::iterator_category{}). For most of those constructors this is a waste of time, because we know the arguments are already iterators. For basic_string(const CharT*) and basic_string(initializer_list) we know that we call _M_construct with two pointers, and for basic_string(const basic_string&) we call it with two const_iterators. Those constructors can call the three-argument overload of _M_construct with the iterator category tag right away, without the intermediate dispatching. The case where this doesn't apply is basic_string(InputIter, InputIter), but for C++11 and later this is constrained so we know it's an iterator here as well. We can restrict the dispatching in this constructor to only be done for C++98 and to call _M_construct_aux directly, which allows us to remove the two-argument _M_construct(InputIter, InputIter) overload entirely. N.B. When calling the three-arg _M_construct with pointers or string iterators, we pass forward_iterator_tag not random_access_iterator_tag. This is because it makes no difference which overload gets called, and simplifies overload resolution to not have to do a base-to-derived check. If we ever add a new overload of M_construct for random access iterators we would have to revisit this, but that seems unlikely. This patch also moves the __is_null_pointer checks from the three-arg _M_construct into the constructors where a null pointer argument is actually possible. This avoids redundant checks where we know we have a non-null pointer, or don't have a pointer at all. Finally, this patch replaces some try-blocks with an RAII type, so that memory is deallocated during unwinding. This avoids the overhead of catching and rethrowing an exception. libstdc++-v3/ChangeLog: * include/bits/basic_string.h (_M_construct_aux): Only define for C++98. Remove constexpr. (_M_construct_aux_2): Likewise. (_M_construct(InputIter, InputIter)): Remove. (basic_string(const basic_string&)): Call _M_construct with iterator category argument. (basic_string(const basic_string&, size_type, const Alloc&)): Likewise. (basic_string(const basic_string&, size_type, size_type)): Likewise. (basic_string(const charT*, size_type, const Alloc&)): Likewise. Check for null pointer. (basic_string(const charT*, const Alloc&)): Likewise. (basic_string(initializer_list, const Alloc&)): Call _M_construct with iterator category argument. (basic_string(const basic_string&, const Alloc&)): Likewise. (basic_string(basic_string&&, const Alloc&)): Likewise. (basic_string(_InputIter, _InputIter, const Alloc&)): Likewise for C++11 and later, call _M_construct_aux for C++98. * include/bits/basic_string.tcc (_M_construct(I, I, input_iterator_tag)): Replace try-block with RAII type. (_M_construct(I, I, forward_iterator_tag)): Likewise. Remove __is_null_pointer check. --- libstdc++-v3/include/bits/basic_string.h | 61 +++++++++++-------- libstdc++-v3/include/bits/basic_string.tcc | 69 ++++++++++++---------- 2 files changed, 74 insertions(+), 56 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 9d281f5daf2..d29c9cdc410 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -262,10 +262,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _M_destroy(size_type __size) throw() { _Alloc_traits::deallocate(_M_get_allocator(), _M_data(), __size + 1); } +#if __cplusplus < 201103L || defined _GLIBCXX_DEFINING_STRING_INSTANTIATIONS // _M_construct_aux is used to implement the 21.3.1 para 15 which // requires special behaviour if _InIterator is an integral type template - _GLIBCXX20_CONSTEXPR void _M_construct_aux(_InIterator __beg, _InIterator __end, std::__false_type) @@ -277,24 +277,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // _GLIBCXX_RESOLVE_LIB_DEFECTS // 438. Ambiguity in the "do the right thing" clause template - _GLIBCXX20_CONSTEXPR void _M_construct_aux(_Integer __beg, _Integer __end, std::__true_type) { _M_construct_aux_2(static_cast(__beg), __end); } - _GLIBCXX20_CONSTEXPR void _M_construct_aux_2(size_type __req, _CharT __c) { _M_construct(__req, __c); } - - template - _GLIBCXX20_CONSTEXPR - void - _M_construct(_InIterator __beg, _InIterator __end) - { - typedef typename std::__is_integer<_InIterator>::__type _Integral; - _M_construct_aux(__beg, __end, _Integral()); - } +#endif // For Input Iterators, used in istreambuf_iterators, etc. template @@ -514,7 +504,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const basic_string& __str) : _M_dataplus(_M_local_data(), _Alloc_traits::_S_select_on_copy(__str._M_get_allocator())) - { _M_construct(__str._M_data(), __str._M_data() + __str.length()); } + { + _M_construct(__str._M_data(), __str._M_data() + __str.length(), + std::forward_iterator_tag()); + } // _GLIBCXX_RESOLVE_LIB_DEFECTS // 2583. no way to supply an allocator for basic_string(str, pos) @@ -531,7 +524,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { const _CharT* __start = __str._M_data() + __str._M_check(__pos, "basic_string::basic_string"); - _M_construct(__start, __start + __str._M_limit(__pos, npos)); + _M_construct(__start, __start + __str._M_limit(__pos, npos), + std::forward_iterator_tag()); } /** @@ -547,7 +541,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { const _CharT* __start = __str._M_data() + __str._M_check(__pos, "basic_string::basic_string"); - _M_construct(__start, __start + __str._M_limit(__pos, __n)); + _M_construct(__start, __start + __str._M_limit(__pos, __n), + std::forward_iterator_tag()); } /** @@ -564,7 +559,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { const _CharT* __start = __str._M_data() + __str._M_check(__pos, "string::string"); - _M_construct(__start, __start + __str._M_limit(__pos, __n)); + _M_construct(__start, __start + __str._M_limit(__pos, __n), + std::forward_iterator_tag()); } /** @@ -580,7 +576,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const _CharT* __s, size_type __n, const _Alloc& __a = _Alloc()) : _M_dataplus(_M_local_data(), __a) - { _M_construct(__s, __s + __n); } + { + // NB: Not required, but considered best practice. + if (__s == 0 && __n > 0) + std::__throw_logic_error(__N("basic_string: " + "construction from null is not valid")); + _M_construct(__s, __s + __n, std::forward_iterator_tag()); + } /** * @brief Construct string as copy of a C string. @@ -596,10 +598,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const _CharT* __s, const _Alloc& __a = _Alloc()) : _M_dataplus(_M_local_data(), __a) { - const _CharT* __end = __s ? __s + traits_type::length(__s) - // We just need a non-null pointer here to get an exception: - : reinterpret_cast(__alignof__(_CharT)); - _M_construct(__s, __end, random_access_iterator_tag()); + // NB: Not required, but considered best practice. + if (__s == 0) + std::__throw_logic_error(__N("basic_string: " + "construction from null is not valid")); + const _CharT* __end = __s + traits_type::length(__s); + _M_construct(__s, __end, forward_iterator_tag()); } /** @@ -657,12 +661,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _GLIBCXX20_CONSTEXPR basic_string(initializer_list<_CharT> __l, const _Alloc& __a = _Alloc()) : _M_dataplus(_M_local_data(), __a) - { _M_construct(__l.begin(), __l.end()); } + { _M_construct(__l.begin(), __l.end(), std::forward_iterator_tag()); } _GLIBCXX20_CONSTEXPR basic_string(const basic_string& __str, const _Alloc& __a) : _M_dataplus(_M_local_data(), __a) - { _M_construct(__str.begin(), __str.end()); } + { _M_construct(__str.begin(), __str.end(), std::forward_iterator_tag()); } _GLIBCXX20_CONSTEXPR basic_string(basic_string&& __str, const _Alloc& __a) @@ -686,7 +690,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 __str._M_set_length(0); } else - _M_construct(__str.begin(), __str.end()); + _M_construct(__str.begin(), __str.end(), std::forward_iterator_tag()); } basic_string(nullptr_t) = delete; @@ -709,7 +713,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(_InputIterator __beg, _InputIterator __end, const _Alloc& __a = _Alloc()) : _M_dataplus(_M_local_data(), __a) - { _M_construct(__beg, __end); } + { +#if __cplusplus >= 201103L + _M_construct(__beg, __end, std::__iterator_category(__beg)); +#else + typedef typename std::__is_integer<_InputIterator>::__type _Integral; + _M_construct_aux(__beg, __end, _Integral()); +#endif + } #if __cplusplus >= 201703L /** diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 5a51f7e21b5..9a54b63b933 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -178,29 +178,34 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION ++__beg; } - __try + struct _Guard + { + _GLIBCXX20_CONSTEXPR + explicit _Guard(basic_string* __s) : _M_guarded(__s) { } + + _GLIBCXX20_CONSTEXPR + ~_Guard() { if (_M_guarded) _M_guarded->_M_dispose(); } + + basic_string* _M_guarded; + } __guard(this); + + while (__beg != __end) { - while (__beg != __end) + if (__len == __capacity) { - if (__len == __capacity) - { - // Allocate more space. - __capacity = __len + 1; - pointer __another = _M_create(__capacity, __len); - this->_S_copy(__another, _M_data(), __len); - _M_dispose(); - _M_data(__another); - _M_capacity(__capacity); - } - _M_data()[__len++] = *__beg; - ++__beg; + // Allocate more space. + __capacity = __len + 1; + pointer __another = _M_create(__capacity, __len); + this->_S_copy(__another, _M_data(), __len); + _M_dispose(); + _M_data(__another); + _M_capacity(__capacity); } + _M_data()[__len++] = *__beg; + ++__beg; } - __catch(...) - { - _M_dispose(); - __throw_exception_again; - } + + __guard._M_guarded = 0; _M_set_length(__len); } @@ -213,11 +218,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_construct(_InIterator __beg, _InIterator __end, std::forward_iterator_tag) { - // NB: Not required, but considered best practice. - if (__gnu_cxx::__is_null_pointer(__beg) && __beg != __end) - std::__throw_logic_error(__N("basic_string::" - "_M_construct null not valid")); - size_type __dnew = static_cast(std::distance(__beg, __end)); if (__dnew > size_type(_S_local_capacity)) @@ -229,13 +229,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_use_local_data(); // Check for out_of_range and length_error exceptions. - __try - { this->_S_copy_chars(_M_data(), __beg, __end); } - __catch(...) - { - _M_dispose(); - __throw_exception_again; - } + struct _Guard + { + _GLIBCXX20_CONSTEXPR + explicit _Guard(basic_string* __s) : _M_guarded(__s) { } + + _GLIBCXX20_CONSTEXPR + ~_Guard() { if (_M_guarded) _M_guarded->_M_dispose(); } + + basic_string* _M_guarded; + } __guard(this); + + this->_S_copy_chars(_M_data(), __beg, __end); + + __guard._M_guarded = 0; _M_set_length(__dnew); }