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 <jwak...@redhat.com>
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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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
// <http://www.gnu.org/licenses/>.
-// 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 <string>
#include <utility>
#include <testsuite_hooks.h>
@@ -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");
