From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id 88315385773B; Thu, 27 Apr 2023 23:04:13 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 88315385773B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1682636653; bh=YMh9l5C+6zz+I9svY7JXLNbmFFzk6k7F24mT3OTn8N8=; h=From:To:Subject:Date:From; b=IgBSg5tpQjk5Ax2bOnFBci0B6bRzaZNxeteljV/Y9HEKCWWBnT5bwdfAV4JI0X3ic NrOI8TVpTmNViQKPLqEc9nzBgVqTcLszjREUQAonKKthsylgcPt7yd+7ThaHo/n82X dWRVaE9RlQUOK8EtSCuCSSuxNe8I7RF0RcUdsGts= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r10-11317] libstdc++: Fix std::common_iterator assignment [PR100823] X-Act-Checkin: gcc X-Git-Author: Jonathan Wakely X-Git-Refname: refs/heads/releases/gcc-10 X-Git-Oldrev: 3cf551240fcbc7a5e0f5ba07a9164e237e6c097b X-Git-Newrev: 5d7de4c8fd9af68011e1fa7bc1d022db81d89594 Message-Id: <20230427230413.88315385773B@sourceware.org> Date: Thu, 27 Apr 2023 23:04:13 +0000 (GMT) List-Id: https://gcc.gnu.org/g:5d7de4c8fd9af68011e1fa7bc1d022db81d89594 commit r10-11317-g5d7de4c8fd9af68011e1fa7bc1d022db81d89594 Author: Jonathan Wakely Date: Wed Jul 20 16:51:44 2022 +0100 libstdc++: Fix std::common_iterator assignment [PR100823] This fixes the following conformance problems reported in the PR: - Move constructor and move assignment should be defined. - Copy assignment from a valueless object should be allowed. Assignment is completely rewritten by this patch, as the previous version had a number of problems. The converting assignment failed to handle the case of assigning a new value to a valueless object, which should work. It only accepted lvalue arguments, so wasn't usable to implement the move assignment operator. Finally, it enforced the precondition that the argument is not valueless, which is correct for the converting assignment but not for the copy assignment. A new _M_assign member is added to handle all cases of assignment (copying from an lvalue, moving from an rvalue, and converting from a different type). The not valueless precondition is checked in the converting assignment before calling _M_assign, so isn't enforced for copy and move assignment. The new function no longer uses a switch, so handles valueless objects as the LHS or RHS of the assignment. libstdc++-v3/ChangeLog: PR libstdc++/100823 * include/bits/stl_iterator.h (common_iterator): Define move constructor and move assignment operator. (common_iterator::_M_assign): New function implementing assignment. (common_iterator::operator=): Use _M_assign. (common_iterator::_S_valueless): New constant. * testsuite/24_iterators/common_iterator/100823.cc: New test. (cherry picked from commit 56c999860bbbb2fd5091ba0985e2e5eaa90c6478) Diff: --- libstdc++-v3/include/bits/stl_iterator.h | 126 ++++++++++++++------- .../24_iterators/common_iterator/100823.cc | 43 +++++++ 2 files changed, 129 insertions(+), 40 deletions(-) diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index ca15fb46cf2..15fedc32d5f 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1753,6 +1753,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION noexcept(_S_noexcept()) : _M_valueless(), _M_index(__x._M_index) { + __glibcxx_assert(__x._M_has_value()); if (_M_index == 0) { if constexpr (is_trivially_default_constructible_v<_It>) @@ -1790,14 +1791,58 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } } + constexpr + common_iterator(common_iterator&& __x) + noexcept(_S_noexcept<_It, _Sent>()) + : _M_valueless(), _M_index(__x._M_index) + { + if (_M_index == 0) + { + if constexpr (is_trivially_default_constructible_v<_It>) + _M_it = std::move(__x._M_it); + else + ::new((void*)std::__addressof(_M_it)) _It(std::move(__x._M_it)); + } + else if (_M_index == 1) + { + if constexpr (is_trivially_default_constructible_v<_Sent>) + _M_sent = std::move(__x._M_sent); + else + ::new((void*)std::__addressof(_M_sent)) + _Sent(std::move(__x._M_sent)); + } + } + + constexpr common_iterator& + operator=(const common_iterator&) = default; + common_iterator& operator=(const common_iterator& __x) noexcept(is_nothrow_copy_assignable_v<_It> && is_nothrow_copy_assignable_v<_Sent> && is_nothrow_copy_constructible_v<_It> && is_nothrow_copy_constructible_v<_Sent>) + requires (!is_trivially_copy_assignable_v<_It> + || !is_trivially_copy_assignable_v<_Sent>) + { + _M_assign(__x); + return *this; + } + + constexpr common_iterator& + operator=(common_iterator&&) = default; + + common_iterator& + operator=(common_iterator&& __x) + noexcept(is_nothrow_move_assignable_v<_It> + && is_nothrow_move_assignable_v<_Sent> + && is_nothrow_move_constructible_v<_It> + && is_nothrow_move_constructible_v<_Sent>) + requires (!is_trivially_move_assignable_v<_It> + || !is_trivially_move_assignable_v<_Sent>) { - return this->operator=<_It, _Sent>(__x); + _M_assign(std::move(__x)); + return *this; } template @@ -1812,48 +1857,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION && is_nothrow_assignable_v<_It&, const _It2&> && is_nothrow_assignable_v<_Sent&, const _Sent2&>) { - switch(_M_index << 2 | __x._M_index) - { - case 0b0000: - _M_it = __x._M_it; - break; - case 0b0101: - _M_sent = __x._M_sent; - break; - case 0b0001: - _M_it.~_It(); - _M_index = -1; - [[fallthrough]]; - case 0b1001: - ::new((void*)std::__addressof(_M_sent)) _Sent(__x._M_sent); - _M_index = 1; - break; - case 0b0100: - _M_sent.~_Sent(); - _M_index = -1; - [[fallthrough]]; - case 0b1000: - ::new((void*)std::__addressof(_M_it)) _It(__x._M_it); - _M_index = 0; - break; - default: - __glibcxx_assert(__x._M_has_value()); - __builtin_unreachable(); - } + __glibcxx_assert(__x._M_has_value()); + _M_assign(__x); return *this; } ~common_iterator() { - switch (_M_index) - { - case 0: - _M_it.~_It(); - break; - case 1: - _M_sent.~_Sent(); - break; - } + if (_M_index == 0) + _M_it.~_It(); + else if (_M_index == 1) + _M_sent.~_Sent(); } decltype(auto) @@ -2007,7 +2021,37 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION requires (!same_as<_It2, _Sent2>) && copyable<_It2> friend class common_iterator; - bool _M_has_value() const noexcept { return _M_index < 2; } + bool + _M_has_value() const noexcept { return _M_index != _S_valueless; } + + template + void + _M_assign(_CIt&& __x) + { + if (_M_index == __x._M_index) + { + if (_M_index == 0) + _M_it = std::forward<_CIt>(__x)._M_it; + else if (_M_index == 1) + _M_sent = std::forward<_CIt>(__x)._M_sent; + } + else + { + if (_M_index == 0) + _M_it.~_It(); + else if (_M_index == 1) + _M_sent.~_Sent(); + _M_index = _S_valueless; + + if (__x._M_index == 0) + ::new((void*)std::__addressof(_M_it)) + _It(std::forward<_CIt>(__x)._M_it); + else if (__x._M_index == 1) + ::new((void*)std::__addressof(_M_sent)) + _Sent(std::forward<_CIt>(__x)._M_sent); + _M_index = __x._M_index; + } + } union { @@ -2015,7 +2059,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Sent _M_sent; unsigned char _M_valueless; }; - unsigned char _M_index; // 0==_M_it, 1==_M_sent, 2==valueless + unsigned char _M_index; // 0 == _M_it, 1 == _M_sent, 2 == valueless + + static constexpr unsigned char _S_valueless{2}; }; template diff --git a/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc b/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc new file mode 100644 index 00000000000..4f2b23de8cc --- /dev/null +++ b/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc @@ -0,0 +1,43 @@ +// { dg-options "-std=gnu++20 -D_GLIBCXX_ASSERTIONS" } +// { dg-do run { target c++20 } } +#include +#include +#include + +void +test_valueless_assignment() +{ + int x[1] { }; + __gnu_test::test_forward_range r(x); + using Iter = decltype(r.begin()); + using Sent = decltype(r.end()); + + std::common_iterator i; + const std::common_iterator j(r.begin()); + try + { + struct Bomb + { + bool operator==(Iter) const { return true; } + operator Sent() const { throw 1; } + }; + std::common_iterator b{Bomb{}}; + i = b; // Throws, leaving i valueless-by-exception. + VERIFY(false); + } + catch (int) + { + std::common_iterator k(i); + + // PR libstdc++/100823 + k = i; // Valid even though both operands are valueless. + + i = j; // No longer valueless. + } + VERIFY( i == j ); +} + +int main() +{ + test_valueless_assignment(); +}