public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [committed 1/3] libstdc++: Fix minor bugs in std::common_iterator
@ 2022-07-20 23:00 Jonathan Wakely
  2022-07-20 23:00 ` [committed 2/3] libstdc++: Fix std::common_iterator assignment [PR100823] Jonathan Wakely
  2022-07-20 23:00 ` [committed 3/3] libstdc++: Fix std::common_iterator triviality [PR100823] Jonathan Wakely
  0 siblings, 2 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-07-20 23:00 UTC (permalink / raw)
  To: libstdc++, gcc-patches

Tested powerpc64le-linux, pushed to trunk.

-- >8 --

The noexcept-specifier for some std::common_iterator constructors was
incorrectly using an rvalue as the first argument of
std::is_nothrow_assignable_v. This gave the wrong answer for some types,
e.g. std::common_iterator<int*, S>, because an rvalue of scalar type
cannot be assigned to.

Also fix the friend declaration to use the same constraints as on the
definition of the class template. G++ fails to diagnose this error, due
to PR c++/96830.

Finally, the copy constructor was using std::move for its argument
in some cases, which should be removed.

libstdc++-v3/ChangeLog:

	* include/bits/stl_iterator.h (common_iterator): Fix incorrect
	uses of is_nothrow_assignable_v. Fix inconsistent constraints on
	friend declaration. Do not move argument in copy constructor.
	* testsuite/24_iterators/common_iterator/1.cc: Check for
	noexcept constructibnle/assignable.
---
 libstdc++-v3/include/bits/stl_iterator.h      | 11 +++++----
 .../24_iterators/common_iterator/1.cc         | 23 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 049cb02a4c4..fd0ae3aa771 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1838,7 +1838,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _S_noexcept1()
       {
 	if constexpr (is_trivially_default_constructible_v<_Tp>)
-	  return is_nothrow_assignable_v<_Tp, _Up>;
+	  return is_nothrow_assignable_v<_Tp&, _Up>;
 	else
 	  return is_nothrow_constructible_v<_Tp, _Up>;
       }
@@ -1932,14 +1932,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       if (_M_index == 0)
 	{
 	  if constexpr (is_trivially_default_constructible_v<_It>)
-	    _M_it = std::move(__x._M_it);
+	    _M_it = __x._M_it;
 	  else
 	    std::construct_at(std::__addressof(_M_it), __x._M_it);
 	}
       else if (_M_index == 1)
 	{
 	  if constexpr (is_trivially_default_constructible_v<_Sent>)
-	    _M_sent = std::move(__x._M_sent);
+	    _M_sent = __x._M_sent;
 	  else
 	    std::construct_at(std::__addressof(_M_sent), __x._M_sent);
 	}
@@ -1964,8 +1964,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       operator=(const common_iterator<_It2, _Sent2>& __x)
       noexcept(is_nothrow_constructible_v<_It, const _It2&>
 	       && is_nothrow_constructible_v<_Sent, const _Sent2&>
-	       && is_nothrow_assignable_v<_It, const _It2&>
-	       && is_nothrow_assignable_v<_Sent, const _Sent2&>)
+	       && is_nothrow_assignable_v<_It&, const _It2&>
+	       && is_nothrow_assignable_v<_Sent&, const _Sent2&>)
       {
 	switch(_M_index << 2 | __x._M_index)
 	  {
@@ -2164,6 +2164,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   private:
     template<input_or_output_iterator _It2, sentinel_for<_It2> _Sent2>
+      requires (!same_as<_It2, _Sent2>) && copyable<_It2>
       friend class common_iterator;
 
     constexpr bool _M_has_value() const noexcept { return _M_index < 2; }
diff --git a/libstdc++-v3/testsuite/24_iterators/common_iterator/1.cc b/libstdc++-v3/testsuite/24_iterators/common_iterator/1.cc
index 365ee89c02e..ec4a86c862a 100644
--- a/libstdc++-v3/testsuite/24_iterators/common_iterator/1.cc
+++ b/libstdc++-v3/testsuite/24_iterators/common_iterator/1.cc
@@ -27,15 +27,30 @@ test01()
   using I = std::common_iterator<int*, const int*>;
   static_assert( std::is_default_constructible_v<I> );
   static_assert( std::is_copy_constructible_v<I> );
+  static_assert( std::is_move_constructible_v<I> );
   static_assert( std::is_copy_assignable_v<I> );
+  static_assert( std::is_move_assignable_v<I> );
   static_assert( std::is_constructible_v<I, int*> );
   static_assert( std::is_constructible_v<I, const int*> );
 
-  struct sentinel { operator int*() const { return nullptr; } };
+  static_assert( std::is_nothrow_copy_constructible_v<I> ); // GCC extension
+  static_assert( std::is_nothrow_move_constructible_v<I> ); // GCC extension
+  static_assert( std::is_nothrow_copy_assignable_v<I> ); // GCC extension
+  static_assert( std::is_nothrow_move_assignable_v<I> ); // GCC extension
+
+  struct sentinel { operator int*() const noexcept { return nullptr; } };
   using K = std::common_iterator<int*, sentinel>;
   static_assert( std::is_constructible_v<I, const K&> );
   static_assert( std::is_assignable_v<I, const K&> );
 
+  static_assert( std::is_nothrow_assignable_v<I&, const K&> ); // GCC extension
+
+  struct sentinel_throwing { operator int*() const { return nullptr; } };
+  using K_throwing = std::common_iterator<int*, sentinel_throwing>;
+  // Conversion is noexcept(false)
+  static_assert( ! std::is_nothrow_assignable_v<I&, const K_throwing&> );
+
+
   struct sentinel2
   {
     const int* p;
@@ -46,6 +61,12 @@ test01()
   using J = std::common_iterator<const int*, sentinel2>;
   static_assert( std::is_constructible_v<J, const I&> );
   static_assert( std::is_convertible_v<const I&, J> );
+
+  static_assert( std::is_constructible_v<J, I> );
+  static_assert( std::is_convertible_v<I, J> );
+
+  // Constructor is noexcept(false)
+  static_assert( ! std::is_nothrow_constructible_v<J, I> );
 }
 
 void
-- 
2.34.3


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [committed 2/3] libstdc++: Fix std::common_iterator assignment [PR100823]
  2022-07-20 23:00 [committed 1/3] libstdc++: Fix minor bugs in std::common_iterator Jonathan Wakely
@ 2022-07-20 23:00 ` Jonathan Wakely
  2022-07-20 23:00 ` [committed 3/3] libstdc++: Fix std::common_iterator triviality [PR100823] Jonathan Wakely
  1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-07-20 23:00 UTC (permalink / raw)
  To: libstdc++, gcc-patches

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [committed 3/3] libstdc++: Fix std::common_iterator triviality [PR100823]
  2022-07-20 23:00 [committed 1/3] libstdc++: Fix minor bugs in std::common_iterator Jonathan Wakely
  2022-07-20 23:00 ` [committed 2/3] libstdc++: Fix std::common_iterator assignment [PR100823] Jonathan Wakely
@ 2022-07-20 23:00 ` Jonathan Wakely
  1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-07-20 23:00 UTC (permalink / raw)
  To: libstdc++, gcc-patches

Tested powerpc64le-linux, pushed to trunk.

-- >8 --

This fixes the remaining problem reported in the PR, that the special
members should be trivial.  This can be done by constraining the
non-trivial versions and adding defaulted overloads that will be used
when the union members are trivial.

Making these members trivial alters the argument passing ABI and so
isn't suitable for backporting to release branches.

libstdc++-v3/ChangeLog:

	PR libstdc++/100823
	* include/bits/stl_iterator.h (common_iterator): Define
	destructor, copy constructor and move constructor as trivial
	when the underlying types allow.
	* testsuite/24_iterators/common_iterator/100823.cc: Check
	triviality of special members.
---
 libstdc++-v3/include/bits/stl_iterator.h          | 15 +++++++++++++++
 .../24_iterators/common_iterator/100823.cc        | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index a913c04deaa..9cd262cd1d9 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1925,9 +1925,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  }
       }
 
+    common_iterator(const common_iterator&) = default;
+
     constexpr
     common_iterator(const common_iterator& __x)
     noexcept(_S_noexcept<const _It&, const _Sent&>())
+    requires (!is_trivially_copyable_v<_It> || !is_trivially_copyable_v<_Sent>)
     : _M_valueless(), _M_index(__x._M_index)
     {
       if (_M_index == 0)
@@ -1946,9 +1949,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
     }
 
+    common_iterator(common_iterator&&) = default;
+
     constexpr
     common_iterator(common_iterator&& __x)
     noexcept(_S_noexcept<_It, _Sent>())
+    requires (!is_trivially_copyable_v<_It> || !is_trivially_copyable_v<_Sent>)
     : _M_valueless(), _M_index(__x._M_index)
     {
       if (_M_index == 0)
@@ -2017,8 +2023,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return *this;
       }
 
+#if __cpp_concepts >= 202002L // Constrained special member functions
+    ~common_iterator() = default;
+
     constexpr
     ~common_iterator()
+      requires (!is_trivially_destructible_v<_It>
+		  || !is_trivially_destructible_v<_Sent>)
+#else
+    constexpr
+    ~common_iterator()
+#endif
     {
       if (_M_index == 0)
 	_M_it.~_It();
diff --git a/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc b/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc
index 4f2b23de8cc..b42dd087ab2 100644
--- a/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc
+++ b/libstdc++-v3/testsuite/24_iterators/common_iterator/100823.cc
@@ -4,6 +4,21 @@
 #include <testsuite_iterators.h>
 #include <testsuite_hooks.h>
 
+void
+test_triviality()
+{
+  using I = std::common_iterator<int*, const int*>;
+
+  // Cannot be trivial, because it has to initialize members.
+  static_assert( ! std::is_trivially_default_constructible_v<I> );
+
+  static_assert( std::is_trivially_destructible_v<I> );
+  static_assert( std::is_trivially_copy_constructible_v<I> );
+  static_assert( std::is_trivially_copy_assignable_v<I> );
+  static_assert( std::is_trivially_move_constructible_v<I> );
+  static_assert( std::is_trivially_move_assignable_v<I> );
+}
+
 void
 test_valueless_assignment()
 {
-- 
2.34.3


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-07-20 23:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 23:00 [committed 1/3] libstdc++: Fix minor bugs in std::common_iterator Jonathan Wakely
2022-07-20 23:00 ` [committed 2/3] libstdc++: Fix std::common_iterator assignment [PR100823] Jonathan Wakely
2022-07-20 23:00 ` [committed 3/3] libstdc++: Fix std::common_iterator triviality [PR100823] Jonathan Wakely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).