From patchwork Thu Oct 25 21:53:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 989388 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-488326-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="X8JhuvBe"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42h19V5CwYz9sCW for ; Fri, 26 Oct 2018 08:54:04 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=Y0UpWzFO0mhKHuJLx1F7t9YyemHUCL+2KvNoSJ2VfMqodI4fAV+N5 XPb2Xl715JTdOesJ4rBQXylVoxXNZquIInoGV1mYWQeGRzy2gg1Y8Fi2v2xC9sqx 8CHd8Ngc43AZ1AmyVZz+0MkQSXQqaWfhNQDpIQH7LQIAg/FncwP47A= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=1fBtSR5UsjODG2fx+gg4mn625No=; b=X8JhuvBeOgkLbX9eVLff 2j8q7YEznixLYgIRzjuFSkHzxa9hzlFX9KQc+NyOyshZeuxA1JBRMrBtycdkOOxI +ig08pnOfoPSxwZCKU/naCabd/Jh069vE4DZHltFyOvkxcgDZIlSFPRrsKvEIZ9u 8QxC1PQ+/QkSlwvLDvAQGVE= Received: (qmail 114525 invoked by alias); 25 Oct 2018 21:53:57 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 114501 invoked by uid 89); 25 Oct 2018 21:53:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=5426, l1, 2630 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 Oct 2018 21:53:53 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7ACABC049AFA; Thu, 25 Oct 2018 21:53:52 +0000 (UTC) Received: from localhost (unknown [10.33.36.86]) by smtp.corp.redhat.com (Postfix) with ESMTP id ECD9C600CC; Thu, 25 Oct 2018 21:53:51 +0000 (UTC) Date: Thu, 25 Oct 2018 22:53:50 +0100 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: RFC: Allow moved-from strings to be non-empty Message-ID: <20181025215349.GA12654@redhat.com> MIME-Version: 1.0 Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.2 (2017-12-15) When an SSO string is contained in the small string buffer or has an unequal allocator a move operation performs a copy, leaving the original data in the moved-from string. Setting the length of the moved-from string to zero is not required, so we can avoid two writes (to the length member and to set the first character to nul) by leaving the moved-from string unchanged. This might be surprising to some users, who (wrongly) expect a string to always be empty after a move. Is that acceptable? It's worth noting that the current behaviour, where we do more work than required, also surprises some people, e.g. https://stackoverflow.com/q/52696413/981959 Some tests need to be adjusted due to the new behaviour, but they have comments saying they're testing specific implementation details, so I don't think needing to adjust them is an argument against making the change: // NOTE: This makes use of the fact that we know how moveable // is implemented on string (via swap). If the implementation changed // this test may begin to fail. Should we make this change? * include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI] (basic_string::basic_string(basic_string&&)): Only set length of source string to zero when allocated storage is transferred. (basic_string::operator=(basic_string&&)): Likewise. Split separate cases into separate conditionals. * testsuite/21_strings/basic_string/cons/char/moveable.cc: Adjust expected state of moved-from strings. * testsuite/21_strings/basic_string/cons/char/moveable2.cc: Likewise. * testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc: Likewise. * testsuite/21_strings/basic_string/modifiers/assign/char/ move_assign.cc: Likewise. * testsuite/21_strings/basic_string/modifiers/assign/wchar_t/ move_assign.cc: Likewise. commit ca3fa69d347c62901eb826f208ec530ea9c70fe7 Author: Jonathan Wakely Date: Thu Oct 25 20:05:12 2018 +0100 Allow moved-from strings to be non-empty When an SSO string is contained in the small string buffer or has an unequal allocator a move operation performs a copy, leaving the original data in the moved-from string. Setting the length of the moved-from string to zero is not required, so we can avoid two writes (to the length member and to set the first character to nul). * include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI] (basic_string::basic_string(basic_string&&)): Only set length of source string to zero when allocated storage is transferred. (basic_string::operator=(basic_string&&)): Likewise. Split separate cases into separate conditionals. * testsuite/21_strings/basic_string/cons/char/moveable.cc: Adjust expected state of moved-from strings. * testsuite/21_strings/basic_string/cons/char/moveable2.cc: Likewise. * testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc: Likewise. * testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc: Likewise. * testsuite/21_strings/basic_string/modifiers/assign/char/ move_assign.cc: Likewise. * testsuite/21_strings/basic_string/modifiers/assign/wchar_t/ move_assign.cc: Likewise. diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index ae6530fcdc9..842e5f012f1 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -542,6 +542,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(basic_string&& __str) noexcept : _M_dataplus(_M_local_data(), std::move(__str._M_get_allocator())) { + // Must use _M_length() here not _M_set_length() because + // basic_stringbuf relies on writing into unallocated capacity so + // we mess up the contents if we put a '\0' in the string. + _M_length(__str.length()); + if (__str._M_is_local()) { traits_type::copy(_M_local_buf, __str._M_local_buf, @@ -551,14 +556,9 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { _M_data(__str._M_data()); _M_capacity(__str._M_allocated_capacity); + __str._M_data(__str._M_local_data()); + __str._M_set_length(0); } - - // Must use _M_length() here not _M_set_length() because - // basic_stringbuf relies on writing into unallocated capacity so - // we mess up the contents if we put a '\0' in the string. - _M_length(__str.length()); - __str._M_data(__str._M_local_data()); - __str._M_set_length(0); } /** @@ -746,7 +746,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 if (__str._M_is_local()) { - // We've always got room for a short string, just copy it. + // We've always got room for a small string, just copy it. if (__str.size()) this->_S_copy(_M_data(), __str._M_data(), __str.size()); _M_set_length(__str.size()); @@ -756,34 +756,38 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 || _M_get_allocator() == __str._M_get_allocator()) { // Just move the allocated pointer, our allocator can free it. - pointer __data = nullptr; - size_type __capacity; - if (!_M_is_local()) + if (_M_is_local()) { - if (_Alloc_traits::_S_always_equal()) - { - // __str can reuse our existing storage. - __data = _M_data(); - __capacity = _M_allocated_capacity; - } - else // __str can't use it, so free it. - _M_destroy(_M_allocated_capacity); + _M_data(__str._M_data()); + _M_length(__str.length()); + _M_capacity(__str._M_allocated_capacity); + __str._M_data(__str._M_local_buf); } - - _M_data(__str._M_data()); - _M_length(__str.length()); - _M_capacity(__str._M_allocated_capacity); - if (__data) + else if (_Alloc_traits::_S_always_equal() + || _M_get_allocator() == __str._M_get_allocator()) { + // __str can reuse our existing storage. + pointer __data = _M_data(); + size_type __capacity = _M_allocated_capacity; + _M_data(__str._M_data()); + _M_length(__str.length()); + _M_capacity(__str._M_allocated_capacity); __str._M_data(__data); __str._M_capacity(__capacity); } else - __str._M_data(__str._M_local_buf); + { + // __str can't use it, so free it. + _M_destroy(_M_allocated_capacity); + _M_data(__str._M_data()); + _M_length(__str.length()); + _M_capacity(__str._M_allocated_capacity); + __str._M_data(__str._M_local_buf); + } + __str.clear(); } else // Need to do a deep copy assign(__str); - __str.clear(); return *this; } diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable.cc index 21932cf7736..e19cd85925b 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable.cc @@ -1,5 +1,4 @@ // { dg-do run { target c++11 } } -// { dg-require-string-conversions "" } // Copyright (C) 2010-2018 Free Software Foundation, Inc. // @@ -18,10 +17,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -31,11 +26,30 @@ void test01() std::string a, b; a.push_back('1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == '1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#else + a.clear(); +#endif + + a.reserve(200); // defeat small string optimization + a.push_back('1'); + b = std::move(a); + VERIFY( b.size() == 1 && b[0] == '1' ); + VERIFY( a.size() == 0 ); // size is unspecified, but true for our string std::string c(std::move(b)); VERIFY( c.size() == 1 && c[0] == '1' ); + VERIFY( b.size() == 0 ); // size is unspecified, but true for our string + + b.push_back('1'); // for SSO string this uses local buffer + std::string d(std::move(b)); + VERIFY( d.size() == 1 && d[0] == '1' ); +#if ! _GLIBCXX_USE_CXX11_ABI VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2.cc index 8488b9698ff..cb4c7f2e39e 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2.cc @@ -19,10 +19,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -40,11 +36,18 @@ void test01() tstring a, b; a.push_back('1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == '1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif tstring c(std::move(b)); VERIFY( c.size() == 1 && c[0] == '1' ); + // b.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc index a6e2ebcade0..706605c5c20 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/char/moveable2_c++17.cc @@ -18,10 +18,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -39,11 +35,18 @@ void test01() tstring a, b; a.push_back('1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == '1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif tstring c(std::move(b)); VERIFY( c.size() == 1 && c[0] == '1' ); + // b.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc index aa6a52749dc..b7ba4865b16 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable.cc @@ -1,5 +1,4 @@ // { dg-do run { target c++11 } } -// { dg-require-string-conversions "" } // Copyright (C) 2010-2018 Free Software Foundation, Inc. // @@ -18,10 +17,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -31,11 +26,30 @@ void test01() std::wstring a, b; a.push_back(L'1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == L'1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == L'1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#else + a.clear(); +#endif + + a.reserve(200); // defeat small string optimization + a.push_back(L'1'); + b = std::move(a); + VERIFY( b.size() == 1 && b[0] == L'1' ); + VERIFY( a.size() == 0 ); // size is unspecified, but true for our string std::wstring c(std::move(b)); VERIFY( c.size() == 1 && c[0] == L'1' ); VERIFY( b.size() == 0 ); + + b.push_back(L'1'); // for SSO string this uses local buffer + std::wstring d(std::move(b)); + VERIFY( d.size() == 1 && d[0] == L'1' ); +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc index 389adc5205d..61d26d85081 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2.cc @@ -1,6 +1,5 @@ // { dg-options "-fno-inline" } // { dg-do run { target c++11 } } -// { dg-require-string-conversions "" } // Copyright (C) 2011-2018 Free Software Foundation, Inc. // @@ -19,10 +18,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -40,11 +35,18 @@ void test01() twstring a, b; a.push_back(L'1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == L'1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == '1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif twstring c(std::move(b)); VERIFY( c.size() == 1 && c[0] == L'1' ); + // b.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc index 3c05a48dd7d..cda7e0f6b96 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/cons/wchar_t/moveable2_c++17.cc @@ -18,10 +18,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changed -// this test may begin to fail. - #include #include #include @@ -39,11 +35,18 @@ void test01() tstring a, b; a.push_back(L'1'); b = std::move(a); - VERIFY( b.size() == 1 && b[0] == L'1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == L'1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif tstring c(std::move(b)); VERIFY( c.size() == 1 && c[0] == L'1' ); + // b.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI VERIFY( b.size() == 0 ); +#endif } int main() diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign.cc b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign.cc index 7089fea04c2..4fbf6bab1cb 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign.cc @@ -1,5 +1,4 @@ // { dg-do run { target c++11 } } -// { dg-require-string-conversions "" } // Copyright (C) 2010-2018 Free Software Foundation, Inc. // @@ -18,10 +17,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changes -// this test may begin to fail. - #include #include #include @@ -31,7 +26,11 @@ void test01() std::string a, b; a.push_back('1'); b.assign(std::move(a)); - VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == '1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif // True for std::allocator because is_always_equal, but not true in general: static_assert(noexcept(a.assign(std::move(b))), "lwg 2063"); diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign.cc b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign.cc index 8d394602a9f..e0550c4f283 100644 --- a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign.cc +++ b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign.cc @@ -1,5 +1,4 @@ // { dg-do run { target c++11 } } -// { dg-require-string-conversions "" } // Copyright (C) 2010-2018 Free Software Foundation, Inc. // @@ -18,10 +17,6 @@ // with this library; see the file COPYING3. If not see // . -// NOTE: This makes use of the fact that we know how moveable -// is implemented on string (via swap). If the implementation changes -// this test may begin to fail. - #include #include #include @@ -31,7 +26,11 @@ void test01() std::wstring a, b; a.push_back(L'1'); b.assign(std::move(a)); - VERIFY( b.size() == 1 && b[0] == '1' && a.size() == 0 ); + VERIFY( b.size() == 1 && b[0] == L'1' ); + // a.size() is unspecified after a move +#if ! _GLIBCXX_USE_CXX11_ABI + VERIFY( a.size() == 0 ); +#endif // True for std::allocator because is_always_equal, but not true in general: static_assert(noexcept(a.assign(std::move(b))), "lwg 2063");