[committed 2/3] libstdc++: Fix std::common_iterator assignment [PR100823]

[Jonathan Wakely <jwakely@redhat.com>]

Tested powerpc64le-linux, pushed to trunk.

-- >8 --

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.
---
 libstdc++-v3/include/bits/stl_iterator.h      | 126 ++++++++++++------
 .../24_iterators/common_iterator/100823.cc    |  43 ++++++
 2 files changed, 129 insertions(+), 40 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index fd0ae3aa771..a913c04deaa 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1908,6 +1908,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       noexcept(_S_noexcept<const _It2&, const _Sent2&>())
       : _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>)
@@ -1945,14 +1946,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
+	    std::construct_at(std::__addressof(_M_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
+	    std::construct_at(std::__addressof(_M_sent),
+			      std::move(__x._M_sent));
+	}
+    }
+
+    constexpr common_iterator&
+    operator=(const common_iterator&) = default;
+
     constexpr 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>)
     {
-      return this->operator=<_It, _Sent>(__x);
+      _M_assign(__x);
+      return *this;
+    }
+
+    constexpr common_iterator&
+    operator=(common_iterator&&) = default;
+
+    constexpr 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>)
+    {
+      _M_assign(std::move(__x));
+      return *this;
     }
 
     template<typename _It2, typename _Sent2>
@@ -1967,49 +2012,18 @@ _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:
-	    std::construct_at(std::__addressof(_M_sent), _Sent(__x._M_sent));
-	    _M_index = 1;
-	    break;
-	  case 0b0100:
-	    _M_sent.~_Sent();
-	    _M_index = -1;
-	    [[fallthrough]];
-	  case 0b1000:
-	    std::construct_at(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;
       }
 
     constexpr
     ~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();
     }
 
     [[nodiscard]]
@@ -2167,7 +2181,37 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       requires (!same_as<_It2, _Sent2>) && copyable<_It2>
       friend class common_iterator;
 
-    constexpr bool _M_has_value() const noexcept { return _M_index < 2; }
+    constexpr bool
+    _M_has_value() const noexcept { return _M_index != _S_valueless; }
+
+    template<typename _CIt>
+      constexpr 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)
+	      std::construct_at(std::__addressof(_M_it),
+				std::forward<_CIt>(__x)._M_it);
+	    else if (__x._M_index == 1)
+	      std::construct_at(std::__addressof(_M_sent),
+				std::forward<_CIt>(__x)._M_sent);
+	    _M_index = __x._M_index;
+	  }
+      }
 
     union
     {
@@ -2175,7 +2219,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<typename _It, typename _Sent>
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 <iterator>
+#include <testsuite_iterators.h>
+#include <testsuite_hooks.h>
+
+void
+test_valueless_assignment()
+{
+  int x[1] { };
+  __gnu_test::test_forward_range<int> r(x);
+  using Iter = decltype(r.begin());
+  using Sent = decltype(r.end());
+
+  std::common_iterator<Iter, Sent> i;
+  const std::common_iterator<Iter, Sent> j(r.begin());
+  try
+  {
+    struct Bomb
+    {
+      bool operator==(Iter) const { return true; }
+      operator Sent() const { throw 1; }
+    };
+    std::common_iterator<Iter, Bomb> b{Bomb{}};
+    i = b; // Throws, leaving i valueless-by-exception.
+    VERIFY(false);
+  }
+  catch (int)
+  {
+    std::common_iterator<Iter, Sent> 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();
+}
-- 
2.34.3