| Message ID | 20181025215349.GA12654@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: Allow moved-from strings to be non-empty | expand |
On Fri, 26 Oct 2018 at 00:54, Jonathan Wakely <jwakely@redhat.com> wrote: > > 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? I would rather not introduce a behavioral difference between us and libc++. It does slightly concern me that some users might actually semantically expect a moved-from string to be empty, even though that's not guaranteed, although for non-SSO cases it *is* guaranteed.
On Thu, 25 Oct 2018, Jonathan Wakely wrote: > 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? I didn't read the patch, but from the description above, I say yes. Interesting that I got my relocate patch in before, because after your patch relocate probably helps a little less than before.
On Fri, 26 Oct 2018, Ville Voutilainen wrote: > I would rather not introduce a behavioral difference between us and > libc++. Why not? There are already several, and it helps find bugs. Maybe you could convince libc++ to change as well if you want to keep the behavior the same? > It does slightly concern me that some users might > actually semantically expect a moved-from string to be empty, even > though that's not guaranteed, although for non-SSO cases > it *is* guaranteed. Is it? In debug mode, I'd be tempted to leave the string as "moved" (size 5, short string so there is no allocation).
On 26/10/18 00:42 +0200, Marc Glisse wrote: >On Fri, 26 Oct 2018, Ville Voutilainen wrote: > >>I would rather not introduce a behavioral difference between us and >>libc++. > >Why not? There are already several, and it helps find bugs. Maybe you >could convince libc++ to change as well if you want to keep the >behavior the same? For the libc++ string zeroing the length of a small string happens to be faster. See Howard's answer at the stackoverflow link I gave. He says: 'So in summary, the setting of the source to an empty string is necessary in "long mode" to transfer ownership of the pointer, and also necessary in short mode for performance reasons to avoid a broken pipeline.' That's not the case for our SSO string, which wasn't designed from the ground up to make this move constructor optimal. We can choose whether to leave moved-from strings empty or not. I'm not persuaded that preserving portable behaviour between implementations is useful here. This is unspecified behaviour. I agree with Marc that introducing a difference helps find bugs, and teaches people not to rely on unspecified properties. I haven't been able to measure any performance difference, and in many cases the writes to the rvalue will be dead stores if the object is about to be destroyed anyway. But the code does look slightly better (caveat: I don't know what I'm talking about). Before: leaq 16(%rdi), %rdx movq %rdi, %rax movq %rdx, (%rdi) movq (%rsi), %rcx leaq 16(%rsi), %rdx cmpq %rdx, %rcx je .L5 movq %rcx, (%rdi) movq 16(%rsi), %rcx movq %rcx, 16(%rdi) .L3: movq 8(%rsi), %rcx movq %rdx, (%rsi) movq $0, 8(%rsi) movq %rcx, 8(%rax) movb $0, 16(%rsi) ret .L5: movdqu 16(%rsi), %xmm0 movups %xmm0, 16(%rdi) jmp .L3 After: leaq 16(%rdi), %rdx movq %rdi, %rax movq %rdx, (%rdi) movq 8(%rsi), %rdx movq (%rsi), %rcx movq %rdx, 8(%rdi) leaq 16(%rsi), %rdx cmpq %rdx, %rcx je .L5 movq %rcx, (%rdi) movq 16(%rsi), %rcx movq %rdx, (%rsi) movq %rcx, 16(%rdi) movq $0, 8(%rsi) movb $0, 16(%rsi) ret .L5: movdqu 16(%rsi), %xmm0 movups %xmm0, 16(%rdi) ret >>It does slightly concern me that some users might >>actually semantically expect a moved-from string to be empty, even >>though that's not guaranteed, although for non-SSO cases >>it *is* guaranteed. > >Is it? In debug mode, I'd be tempted to leave the string as "moved" >(size 5, short string so there is no allocation). That's not a bad idea.
On 26/10/18 00:17 +0100, Jonathan Wakely wrote: >On 26/10/18 00:42 +0200, Marc Glisse wrote: >>On Fri, 26 Oct 2018, Ville Voutilainen wrote: >> >>>I would rather not introduce a behavioral difference between us and >>>libc++. >> >>Why not? There are already several, and it helps find bugs. Maybe >>you could convince libc++ to change as well if you want to keep the >>behavior the same? > >For the libc++ string zeroing the length of a small string happens to >be faster. See Howard's answer at the stackoverflow link I gave. He >says: > >'So in summary, the setting of the source to an empty string is necessary in "long mode" to transfer ownership of the pointer, and also necessary in short mode for performance reasons to avoid a broken pipeline.' > >That's not the case for our SSO string, which wasn't designed from the >ground up to make this move constructor optimal. We can choose whether >to leave moved-from strings empty or not. > >I'm not persuaded that preserving portable behaviour between >implementations is useful here. This is unspecified behaviour. I agree >with Marc that introducing a difference helps find bugs, and teaches >people not to rely on unspecified properties. > >I haven't been able to measure any performance difference, and in many >cases the writes to the rvalue will be dead stores if the object is >about to be destroyed anyway. But the code does look slightly better >(caveat: I don't know what I'm talking about). > >Before: > > leaq 16(%rdi), %rdx > movq %rdi, %rax > movq %rdx, (%rdi) > movq (%rsi), %rcx > leaq 16(%rsi), %rdx > cmpq %rdx, %rcx > je .L5 > movq %rcx, (%rdi) > movq 16(%rsi), %rcx > movq %rcx, 16(%rdi) >.L3: > movq 8(%rsi), %rcx > movq %rdx, (%rsi) > movq $0, 8(%rsi) > movq %rcx, 8(%rax) > movb $0, 16(%rsi) > ret >.L5: > movdqu 16(%rsi), %xmm0 > movups %xmm0, 16(%rdi) > jmp .L3 > > >After: > > leaq 16(%rdi), %rdx > movq %rdi, %rax > movq %rdx, (%rdi) > movq 8(%rsi), %rdx > movq (%rsi), %rcx > movq %rdx, 8(%rdi) > leaq 16(%rsi), %rdx > cmpq %rdx, %rcx > je .L5 > movq %rcx, (%rdi) > movq 16(%rsi), %rcx > movq %rdx, (%rsi) > movq %rcx, 16(%rdi) > movq $0, 8(%rsi) > movb $0, 16(%rsi) > ret >.L5: > movdqu 16(%rsi), %xmm0 > movups %xmm0, 16(%rdi) > ret > > >>>It does slightly concern me that some users might >>>actually semantically expect a moved-from string to be empty, even >>>though that's not guaranteed, although for non-SSO cases >>>it *is* guaranteed. >> >>Is it? In debug mode, I'd be tempted to leave the string as "moved" >>(size 5, short string so there is no allocation). > >That's not a bad idea. Although we can't do it for std::wstring and std::u32string, as their small string buffer is *very* small.
On Fri, 26 Oct 2018, Jonathan Wakely wrote: >> For the libc++ string zeroing the length of a small string happens to >> be faster. Ah, yes, of course. >>> Is it? In debug mode, I'd be tempted to leave the string as "moved" (size >>> 5, short string so there is no allocation). >> >> That's not a bad idea. > > Although we can't do it for std::wstring and std::u32string, as their > small string buffer is *very* small. "N/A"? The proposition was only semi-serious. By the way, I was surprised by the formula for the size of the buffer. It often has size 16, but for a _CharT of size 3 and alignment 1 (unlikely I guess), it has size 18.
On Fri, 26 Oct 2018 at 01:42, Marc Glisse <marc.glisse@inria.fr> wrote: > > On Fri, 26 Oct 2018, Ville Voutilainen wrote: > > > I would rather not introduce a behavioral difference between us and > > libc++. > > Why not? There are already several, and it helps find bugs. Maybe you > could convince libc++ to change as well if you want to keep the behavior > the same? What bugs? > > It does slightly concern me that some users might > > actually semantically expect a moved-from string to be empty, even > > though that's not guaranteed, although for non-SSO cases > > it *is* guaranteed. > > Is it? In debug mode, I'd be tempted to leave the string as "moved" (size > 5, short string so there is no allocation). Sigh. Apparently it isn't, because the standard doesn't bother placing complexity requirements on string constructors. Even so, I'd prefer string acting like vector, so that it will leave the source of a move in an empty state, rather than an unspecified state. Despite the standard not requiring that, it's more useful programmatically to have the empty state than the unspecified state, especially when the state is empty in some cases anyway.
On 26/10/18 12:16 +0300, Ville Voutilainen wrote: >On Fri, 26 Oct 2018 at 01:42, Marc Glisse <marc.glisse@inria.fr> wrote: >> >> On Fri, 26 Oct 2018, Ville Voutilainen wrote: >> >> > I would rather not introduce a behavioral difference between us and >> > libc++. >> >> Why not? There are already several, and it helps find bugs. Maybe you >> could convince libc++ to change as well if you want to keep the behavior >> the same? > >What bugs? Assuming the string is empty after a move and appending to it without calling clear() first. >> > It does slightly concern me that some users might >> > actually semantically expect a moved-from string to be empty, even >> > though that's not guaranteed, although for non-SSO cases >> > it *is* guaranteed. >> >> Is it? In debug mode, I'd be tempted to leave the string as "moved" (size >> 5, short string so there is no allocation). > >Sigh. Apparently it isn't, because the standard doesn't bother placing >complexity >requirements on string constructors. Writing 5 bytes into the SSO buffer would be constant complexity :-P >Even so, I'd prefer string acting >like vector, >so that it will leave the source of a move in an empty state, rather >than an unspecified >state. It's not guaranteed for vector either. An allocator with POCMA==false doesn't propagate on move assignment and if the source object's allocator isn't equal to the target's it will copy, and there's no guarantee the source will be empty. This would be a conforming change to our vector: --- a/libstdc++-v3/include/bits/stl_vector.h +++ b/libstdc++-v3/include/bits/stl_vector.h @@ -1793,7 +1793,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER // so we need to individually move each element. this->assign(std::__make_move_if_noexcept_iterator(__x.begin()), std::__make_move_if_noexcept_iterator(__x.end())); - __x.clear(); } } #endif That might even have a bigger performance benefit, because clearing a vector runs destructors, it doesn't just set the length and write a null terminator. If you're using a vector as a buffer of objects and the first thing you do after v1=std::move(v2) is v2.resize(n) then it's a pessimization to have cleared it. >Despite the standard not requiring that, it's more useful >programmatically >to have the empty state than the unspecified state, especially when the state >is empty in some cases anyway.
On 26/10/18 08:25 +0200, Marc Glisse wrote: >On Fri, 26 Oct 2018, Jonathan Wakely wrote: > >>>For the libc++ string zeroing the length of a small string happens to >>>be faster. > >Ah, yes, of course. > >>>>Is it? In debug mode, I'd be tempted to leave the string as >>>>"moved" (size 5, short string so there is no allocation). >>> >>>That's not a bad idea. >> >>Although we can't do it for std::wstring and std::u32string, as their >>small string buffer is *very* small. > >"N/A"? The proposition was only semi-serious. > >By the way, I was surprised by the formula for the size of the buffer. >It often has size 16, but for a _CharT of size 3 and alignment 1 >(unlikely I guess), it has size 18. Oops, that's not intentional.
On Fri, 26 Oct 2018 at 12:55, Jonathan Wakely <jwakely@redhat.com> wrote: > >> Why not? There are already several, and it helps find bugs. Maybe you > >> could convince libc++ to change as well if you want to keep the behavior > >> the same? > > > >What bugs? > > Assuming the string is empty after a move and appending to it without > calling clear() first. I find it bloody unfortunate that the standard considers that a bug. It seems quite convenient to me that it's possible to have a bag of strings, move some of them out to be processed, and be able to tell the processed ones from the unprocessed ones without having to separately clear them when moving. > >> > It does slightly concern me that some users might > >> > actually semantically expect a moved-from string to be empty, even > >> > though that's not guaranteed, although for non-SSO cases > >> > it *is* guaranteed. > >> > >> Is it? In debug mode, I'd be tempted to leave the string as "moved" (size > >> 5, short string so there is no allocation). > > > >Sigh. Apparently it isn't, because the standard doesn't bother placing > >complexity > >requirements on string constructors. > > Writing 5 bytes into the SSO buffer would be constant complexity :-P Indeed it would, but the standard doesn't seem to have complexity requirements on string constructors at all. If it did, moving a non-sso string would *have to* steal from the source, and the sso case would not have to do that, but could. > >Even so, I'd prefer string acting > >like vector, > >so that it will leave the source of a move in an empty state, rather > >than an unspecified > >state. > > It's not guaranteed for vector either. An allocator with POCMA==false > doesn't propagate on move assignment and if the source object's > allocator isn't equal to the target's it will copy, and there's no > guarantee the source will be empty. Right, I was talking about homogeneous vectors; it is well-known that non-propagating allocators don't move. Except when they do (on move construction).
On Fri, 26 Oct 2018, Ville Voutilainen wrote: > On Fri, 26 Oct 2018 at 12:55, Jonathan Wakely <jwakely@redhat.com> wrote: >>>> Why not? There are already several, and it helps find bugs. Maybe you >>>> could convince libc++ to change as well if you want to keep the behavior >>>> the same? >>> >>> What bugs? >> >> Assuming the string is empty after a move and appending to it without >> calling clear() first. > > I find it bloody unfortunate that the standard considers that a bug. It > seems quite convenient to me that it's possible to have a bag of > strings, move some of them out to be processed, and be able to tell the > processed ones from the unprocessed ones without having to separately > clear them when moving. We all seem to want different things from move: 1) copy, possibly with a speed up because I don't care what the source looks like afterwards. That's what was standardized, and for a small string that means that moving it should just copy. 2) move and clear 3) move and destroy (most likely many others) The convenience does not seem that great to me, especially if it has a performance cost. >>>>> It does slightly concern me that some users might >>>>> actually semantically expect a moved-from string to be empty, even >>>>> though that's not guaranteed, although for non-SSO cases >>>>> it *is* guaranteed. >>>> >>>> Is it? In debug mode, I'd be tempted to leave the string as "moved" (size >>>> 5, short string so there is no allocation). >>> >>> Sigh. Apparently it isn't, because the standard doesn't bother placing >>> complexity >>> requirements on string constructors. >> >> Writing 5 bytes into the SSO buffer would be constant complexity :-P > > Indeed it would, but the standard doesn't seem to have complexity > requirements on string constructors at all. If it did, moving a non-sso > string would *have to* steal from the source, and the sso case would not > have to do that, but could. I am not sure what you are saying exactly, but I think "noexcept" is (kind of) playing the role of a complexity requirement here.
The reason move constructors were introduced was to speed up code in cases where an object Is copied and the copy is no longer needed. It is unfortunate that there may now be code out there that relies on accidental properties of library implementations. It would be best if the Implementation is not constrained. Unless the standard mandates that, after a string is moved, the string is empty, the user should only be able to assume that it is in some consistent but unspecified state. Otherwise we pay a performance penalty forever. If the standard explicitly states that the argument to the move constructor is defined to be empty after the call, we're stuck.
On Sat, 27 Oct 2018 at 01:27, Joe Buck <joe.buck@synopsys.com> wrote: > > The reason move constructors were introduced was to speed up code in cases where an object > Is copied and the copy is no longer needed. It is unfortunate that there may now be code out > there that relies on accidental properties of library implementations. It would be best if the > Implementation is not constrained. Unless the standard mandates that, after a string is moved, > the string is empty, the user should only be able to assume that it is in some consistent but > unspecified state. Otherwise we pay a performance penalty forever. > > If the standard explicitly states that the argument to the move constructor is defined to be > empty after the call, we're stuck. It certainly doesn't specify so for an SSO string, so we're not stuck. On the other hand, we already get a speed-up, it's just not as much of a speed-up as it can be. What I really loathe is the potential implementation divergence; it's all good for the priesthood to refer to the standard and say "you shouldn't have done that", but that's, as a good friend of mine provided as a phrasing on a different manner, spectacularly unhelpful.
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");
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 <jwakely@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.