From 63aeb77ce1c5e389819bb6c0b83fcbd7dd8efe4b Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Thu, 25 Jan 2024 07:55:46 +0800 Subject: [PATCH] ``: Don't swap `basic_stringbuf`s in move assignment and allocator-extended construction (#4239) Co-authored-by: Stephan T. Lavavej --- stl/inc/sstream | 134 ++++++++++++------ tests/libcxx/expected_results.txt | 3 - .../GH_001858_iostream_exception/test.cpp | 25 ++++ .../test.cpp | 17 +++ 4 files changed, 136 insertions(+), 43 deletions(-) diff --git a/stl/inc/sstream b/stl/inc/sstream index 59f0e6b6c0..2a0ae5136d 100644 --- a/stl/inc/sstream +++ b/stl/inc/sstream @@ -66,24 +66,87 @@ public: const basic_string<_Elem, _Traits, _Alloc2>& _Str, ios_base::openmode _Mode = ios_base::in | ios_base::out) : basic_stringbuf(_Str, _Mode, _Alloc{}) {} - basic_stringbuf(basic_stringbuf&& _Right, const _Alloc& _Al_) : _Mystate(0), _Al(_Al_) { - _Assign_rv(_STD move(_Right)); + basic_stringbuf(basic_stringbuf&& _Right, const _Alloc& _Al_) : _Seekhigh(nullptr), _Mystate(0), _Al(_Al_) { + if constexpr (!allocator_traits<_Alloc>::is_always_equal::value) { + if (_Al != _Right._Al) { + _Copy_into_self_and_tidy(_STD move(_Right)); + return; + } + } + + _Take_contents(_STD move(_Right)); } #endif // _HAS_CXX20 - basic_stringbuf(basic_stringbuf&& _Right) : _Mystate(0) { - _Assign_rv(_STD move(_Right)); + basic_stringbuf(basic_stringbuf&& _Right) + : _Seekhigh(_STD exchange(_Right._Seekhigh, nullptr)), _Mystate(_STD exchange(_Right._Mystate, 0)), + _Al(_Right._Al) { + _Mysb::swap(_Right); } - basic_stringbuf& operator=(basic_stringbuf&& _Right) noexcept /* strengthened */ { - _Assign_rv(_STD move(_Right)); + basic_stringbuf& operator=(basic_stringbuf&& _Right) noexcept( + _Choose_pocma_v<_Alloc> != _Pocma_values::_No_propagate_allocators) /* strengthened */ { + if (this != _STD addressof(_Right)) { + _Assign_rv_no_alias(_STD move(_Right)); + } return *this; } - void _Assign_rv(basic_stringbuf&& _Right) noexcept { - if (this != _STD addressof(_Right)) { - _Tidy(); - this->swap(_Right); + void _Take_contents(basic_stringbuf&& _Right) noexcept { + // pre: *this holds no dynamic buffer and _Al == _Right._Al + _Seekhigh = _STD exchange(_Right._Seekhigh, nullptr); + _Mystate = _STD exchange(_Right._Mystate, 0); + _Mysb::swap(_Right); + } + + void _Copy_into_self_and_tidy(basic_stringbuf&& _Right) { + // pre: *this holds no dynamic buffer and _Al != _Right._Al + if ((_Right._Mystate & _Allocated) == 0) { + return; + } + + const auto _Right_data = _Right._Mysb::eback(); + const auto _Right_capacity = static_cast::size_type>( + (_Right._Mysb::pptr() ? _Right._Mysb::epptr() : _Right._Mysb::egptr()) - _Right_data); + + auto _New_capacity = _Right_capacity; + const auto _New_data = _STD _Unfancy(_STD _Allocate_at_least_helper(_Al, _New_capacity)); + + _Mysb::setg(_New_data, _New_data + (_Right._Mysb::gptr() - _Right_data), + _New_data + (_Right._Mysb::egptr() - _Right_data)); + if (_Right._Mysb::pbase() != nullptr) { + const auto _Pbase_diff = _Right._Mysb::pbase() - _Right_data; + const auto _Pptr_diff = _Right._Mysb::pptr() - _Right_data; + const auto _Epptr_diff = _Right._Mysb::epptr() - _Right_data; + _Mysb::setp(_New_data + _Pbase_diff, _New_data + _Pptr_diff, _New_data + _Epptr_diff); + } else { + _Mysb::setp(nullptr, nullptr, nullptr); + } + + const auto _Right_view = _Right._Get_buffer_view(); + if (_Right_view._Ptr != nullptr) { + _Traits::copy(_New_data + (_Right_view._Ptr - _Right_data), _Right_view._Ptr, _Right_view._Size); + } + + _Seekhigh = _New_data + _New_capacity; + _Mystate = _Right._Mystate; + + _Right._Tidy(); + } + + void _Assign_rv_no_alias(basic_stringbuf&& _Right) noexcept( + _Choose_pocma_v<_Alloc> != _Pocma_values::_No_propagate_allocators) { + // pre: this != std::addressof(_Right) + _Tidy(); + if constexpr (_Choose_pocma_v<_Alloc> == _Pocma_values::_No_propagate_allocators) { + if (_Al == _Right._Al) { + _Take_contents(_STD move(_Right)); + } else { + _Copy_into_self_and_tidy(_STD move(_Right)); + } + } else { + _Pocma(_Al, _Right._Al); + _Take_contents(_STD move(_Right)); } } @@ -584,20 +647,17 @@ public: : _Mybase(_STD addressof(_Stringbuffer)), _Stringbuffer(_Str, _Mode | ios_base::in) {} #endif // _HAS_CXX20 - basic_istringstream(basic_istringstream&& _Right) : _Mybase(_STD addressof(_Stringbuffer)) { - _Assign_rv(_STD move(_Right)); - } + basic_istringstream(basic_istringstream&& _Right) + : _Mybase(_STD addressof(_Stringbuffer)), + _Stringbuffer((_Mybase::swap(_Right), _STD move(_Right._Stringbuffer))) {} - basic_istringstream& operator=(basic_istringstream&& _Right) noexcept /* strengthened */ { - _Assign_rv(_STD move(_Right)); - return *this; - } - - void _Assign_rv(basic_istringstream&& _Right) noexcept { + basic_istringstream& operator=(basic_istringstream&& _Right) noexcept( + _Choose_pocma_v<_Alloc> != _Pocma_values::_No_propagate_allocators) /* strengthened */ { if (this != _STD addressof(_Right)) { _Mybase::swap(_Right); - _Stringbuffer._Assign_rv(_STD move(_Right._Stringbuffer)); + _Stringbuffer._Assign_rv_no_alias(_STD move(_Right._Stringbuffer)); } + return *this; } void swap(basic_istringstream& _Right) noexcept /* strengthened */ { @@ -704,20 +764,17 @@ public: : _Mybase(_STD addressof(_Stringbuffer)), _Stringbuffer(_Str, _Mode | ios_base::out) {} #endif // _HAS_CXX20 - basic_ostringstream(basic_ostringstream&& _Right) : _Mybase(_STD addressof(_Stringbuffer)) { - _Assign_rv(_STD move(_Right)); - } + basic_ostringstream(basic_ostringstream&& _Right) + : _Mybase(_STD addressof(_Stringbuffer)), + _Stringbuffer((_Mybase::swap(_Right), _STD move(_Right._Stringbuffer))) {} - basic_ostringstream& operator=(basic_ostringstream&& _Right) noexcept /* strengthened */ { - _Assign_rv(_STD move(_Right)); - return *this; - } - - void _Assign_rv(basic_ostringstream&& _Right) noexcept { + basic_ostringstream& operator=(basic_ostringstream&& _Right) noexcept( + _Choose_pocma_v<_Alloc> != _Pocma_values::_No_propagate_allocators) /* strengthened */ { if (this != _STD addressof(_Right)) { _Mybase::swap(_Right); - _Stringbuffer._Assign_rv(_STD move(_Right._Stringbuffer)); + _Stringbuffer._Assign_rv_no_alias(_STD move(_Right._Stringbuffer)); } + return *this; } void swap(basic_ostringstream& _Right) noexcept /* strengthened */ { @@ -830,20 +887,17 @@ public: : _Mybase(_STD addressof(_Stringbuffer)), _Stringbuffer(_Str, _Mode) {} #endif // _HAS_CXX20 - basic_stringstream(basic_stringstream&& _Right) : _Mybase(_STD addressof(_Stringbuffer)) { - _Assign_rv(_STD move(_Right)); - } + basic_stringstream(basic_stringstream&& _Right) + : _Mybase(_STD addressof(_Stringbuffer)), + _Stringbuffer((_Mybase::swap(_Right), _STD move(_Right._Stringbuffer))) {} - basic_stringstream& operator=(basic_stringstream&& _Right) noexcept /* strengthened */ { - _Assign_rv(_STD move(_Right)); - return *this; - } - - void _Assign_rv(basic_stringstream&& _Right) noexcept { + basic_stringstream& operator=(basic_stringstream&& _Right) noexcept( + _Choose_pocma_v<_Alloc> != _Pocma_values::_No_propagate_allocators) /* strengthened */ { if (this != _STD addressof(_Right)) { _Mybase::swap(_Right); - _Stringbuffer._Assign_rv(_STD move(_Right._Stringbuffer)); + _Stringbuffer._Assign_rv_no_alias(_STD move(_Right._Stringbuffer)); } + return *this; } void swap(basic_stringstream& _Right) noexcept /* strengthened */ { diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt index 615c255e33..ed3fcb4952 100644 --- a/tests/libcxx/expected_results.txt +++ b/tests/libcxx/expected_results.txt @@ -513,9 +513,6 @@ std/algorithms/robust_against_adl.compile.pass.cpp FAIL # GH-3100: etc.: ADL should be avoided when calling _Construct_in_place and its friends std/utilities/function.objects/func.wrap/func.wrap.func/robust_against_adl.pass.cpp FAIL -# GH-4232: : basic_stringbuf shouldn't implement moving with swapping -std/input.output/string.streams/stringbuf/stringbuf.cons/move.alloc.pass.cpp FAIL - # GH-4238: : file_clock::to_utc() overflows for file_time # This test also has a bogus assumption about the file_time epoch, and file_time is doomed on Windows. std/time/time.clock/time.clock.file/ostream.pass.cpp FAIL diff --git a/tests/std/tests/GH_001858_iostream_exception/test.cpp b/tests/std/tests/GH_001858_iostream_exception/test.cpp index 5ddb356a1e..9b9bc47ef2 100644 --- a/tests/std/tests/GH_001858_iostream_exception/test.cpp +++ b/tests/std/tests/GH_001858_iostream_exception/test.cpp @@ -15,6 +15,10 @@ #include #include +#if _HAS_CXX17 +#include +#endif // _HAS_CXX17 + #if _HAS_CXX20 #include #endif // _HAS_CXX20 @@ -322,6 +326,27 @@ STATIC_ASSERT(is_nothrow_move_assignable_v); STATIC_ASSERT(is_nothrow_move_assignable_v); STATIC_ASSERT(is_nothrow_move_assignable_v); +// GH-4232: : basic_stringbuf shouldn't implement moving with swapping +// These operations need to be able to throw exceptions when the source and target allocators are unequal. +#if _HAS_CXX17 +STATIC_ASSERT( + !is_nothrow_move_assignable_v, pmr::polymorphic_allocator>>); +STATIC_ASSERT( + !is_nothrow_move_assignable_v, pmr::polymorphic_allocator>>); +STATIC_ASSERT( + !is_nothrow_move_assignable_v, pmr::polymorphic_allocator>>); +STATIC_ASSERT(!is_nothrow_move_assignable_v< + basic_istringstream, pmr::polymorphic_allocator>>); +STATIC_ASSERT( + !is_nothrow_move_assignable_v, pmr::polymorphic_allocator>>); +STATIC_ASSERT(!is_nothrow_move_assignable_v< + basic_ostringstream, pmr::polymorphic_allocator>>); +STATIC_ASSERT( + !is_nothrow_move_assignable_v, pmr::polymorphic_allocator>>); +STATIC_ASSERT(!is_nothrow_move_assignable_v< + basic_stringstream, pmr::polymorphic_allocator>>); +#endif // _HAS_CXX17 + STATIC_ASSERT(is_nothrow_std_swappable); STATIC_ASSERT(is_nothrow_std_swappable); STATIC_ASSERT(is_nothrow_std_swappable); diff --git a/tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp b/tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp index 421f9ec0f9..a4fc8a0bb3 100644 --- a/tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp +++ b/tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -116,6 +117,22 @@ struct test_pmr_allocator { } Stream stream2{init_value, init_value.get_allocator()}; assert(stream2.rdbuf()->get_allocator() == init_value.get_allocator()); + + // GH-4232: : basic_stringbuf shouldn't implement moving with swapping + + // Move to another stream buffer with different allocators + using SBuf = remove_pointer_t; + SBuf& sbuf = *stream.rdbuf(); + SBuf sbuf2{move(sbuf), init_value.get_allocator()}; + assert(sbuf2.get_allocator() != sbuf.get_allocator()); + assert(stream.view().empty()); + assert(sbuf2.view() == init_value); + + // Move assignment between different memory resources + sbuf = move(sbuf2); + assert(sbuf2.get_allocator() != sbuf.get_allocator()); + assert(sbuf2.view().empty()); + assert(stream.view() == init_value); } };