diff mbox series

libstdc++: Back out some changes in P2325R3 backport [PR103904]

Message ID 20220211165157.3159659-1-ppalka@redhat.com
State New
Headers show
Series libstdc++: Back out some changes in P2325R3 backport [PR103904] | expand

Commit Message

Patrick Palka Feb. 11, 2022, 4:51 p.m. UTC 
In the P2325R3 backport r11-9555 the relaxation of the constraints on
the partial specialization of __box (which is semantically equivalent to
the primary template, only more space efficient) means some
specializations of __box will now use the partial specialization instead
of the primary template, which (IIUC) constitutes an ABI change unsuitable
for a release branch.  This patch reverts this constraint change, which
isn't needed for correctness anyway.

Similarly the change to use __non_propagating_cache for the data member
split_view::_M_current (so that it's always default-initializable) also
constitutes an unsuitable ABI change.  This patch reverts this change
too, and instead further constrains split_view's default constructor to
require that we can default-initialize _M_current.

	PR libstdc++/103904

libstdc++-v3/ChangeLog:

	* include/std/ranges (__detail::__box): Revert r11-9555 changes
	to the constraints on the partial specialization and the
	now-unnecessary member additions.
	(__detail::__non_propagating_cache::operator=): Remove
	now-unused overload added by r11-9555.
	(split_view::_OuterIter::__current): Adjust after reverting the
	r11-9555 change to the type of _M_current.
	(split_view::_M_current): Revert r11-9555 change to its type.
	(split_view::split_view): Constrain the default ctor further.
	* testsuite/std/ranges/adaptors/detail/copyable_box.cc: Disable
	now-irrelevant test for the r11-9555 changes to the partial
	specialization of __box.
---
 libstdc++-v3/include/std/ranges               | 54 +++----------------
 .../ranges/adaptors/detail/copyable_box.cc    |  4 ++
 2 files changed, 10 insertions(+), 48 deletions(-)

Comments

Patrick Palka Feb. 11, 2022, 5:09 p.m. UTC | #1 
On Fri, 11 Feb 2022, Patrick Palka wrote:

> In the P2325R3 backport r11-9555 the relaxation of the constraints on
> the partial specialization of __box (which is semantically equivalent to
> the primary template, only more space efficient) means some
> specializations of __box will now use the partial specialization instead
> of the primary template, which (IIUC) constitutes an ABI change unsuitable
> for a release branch.  This patch reverts this constraint change, which
> isn't needed for correctness anyway.
> 
> Similarly the change to use __non_propagating_cache for the data member
> split_view::_M_current (so that it's always default-initializable) also
> constitutes an unsuitable ABI change.  This patch reverts this change
> too, and instead further constrains split_view's default constructor to
> require that we can default-initialize _M_current.

Forgot to clarify that this is for the 11 branch, tested on
x86_64-pc-linux-gnu.  Does this look reasonable?  I noticed these
issues while backporting r11-9555 to the 10 branch, which doesn't have
__non_propagating_cache or the partial specialization of __box.

> 
> 	PR libstdc++/103904
> 
> libstdc++-v3/ChangeLog:
> 
> 	* include/std/ranges (__detail::__box): Revert r11-9555 changes
> 	to the constraints on the partial specialization and the
> 	now-unnecessary member additions.
> 	(__detail::__non_propagating_cache::operator=): Remove
> 	now-unused overload added by r11-9555.
> 	(split_view::_OuterIter::__current): Adjust after reverting the
> 	r11-9555 change to the type of _M_current.
> 	(split_view::_M_current): Revert r11-9555 change to its type.
> 	(split_view::split_view): Constrain the default ctor further.
> 	* testsuite/std/ranges/adaptors/detail/copyable_box.cc: Disable
> 	now-irrelevant test for the r11-9555 changes to the partial
> 	specialization of __box.
> ---
>  libstdc++-v3/include/std/ranges               | 54 +++----------------
>  .../ranges/adaptors/detail/copyable_box.cc    |  4 ++
>  2 files changed, 10 insertions(+), 48 deletions(-)
> 
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 03c6275801f..bf31e4be500 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -144,8 +144,7 @@ namespace ranges
>      // std::optional.  It provides just the subset of the primary template's
>      // API that we currently use.
>      template<__boxable _Tp>
> -      requires copyable<_Tp> || (is_nothrow_move_constructible_v<_Tp>
> -				 && is_nothrow_copy_constructible_v<_Tp>)
> +      requires copyable<_Tp>
>        struct __box<_Tp>
>        {
>        private:
> @@ -174,38 +173,6 @@ namespace ranges
>  	  : _M_value(std::forward<_Args>(__args)...)
>  	  { }
>  
> -	__box(const __box&) = default;
> -	__box(__box&&) = default;
> -	__box& operator=(const __box&) requires copyable<_Tp> = default;
> -	__box& operator=(__box&&) requires copyable<_Tp> = default;
> -
> -	// When _Tp is nothrow_copy_constructible but not copy_assignable,
> -	// copy assignment is implemented via destroy-then-copy-construct.
> -	constexpr __box&
> -	operator=(const __box& __that) noexcept
> -	{
> -	  static_assert(is_nothrow_copy_constructible_v<_Tp>);
> -	  if (this != std::__addressof(__that))
> -	    {
> -	      _M_value.~_Tp();
> -	      std::construct_at(std::__addressof(_M_value), *__that);
> -	    }
> -	  return *this;
> -	}
> -
> -	// Likewise for move assignment.
> -	constexpr __box&
> -	operator=(__box&& __that) noexcept
> -	{
> -	  static_assert(is_nothrow_move_constructible_v<_Tp>);
> -	  if (this != std::__addressof(__that))
> -	    {
> -	      _M_value.~_Tp();
> -	      std::construct_at(std::__addressof(_M_value), std::move(*__that));
> -	    }
> -	  return *this;
> -	}
> -
>  	constexpr bool
>  	has_value() const noexcept
>  	{ return true; };
> @@ -1180,16 +1147,6 @@ namespace views::__adaptor
>  	  return *this;
>  	}
>  
> -	constexpr __non_propagating_cache&
> -	operator=(_Tp __val)
> -	{
> -	  this->_M_reset();
> -	  std::construct_at(std::__addressof(this->_M_payload._M_payload),
> -			    std::in_place, std::move(__val));
> -	  this->_M_payload._M_engaged = true;
> -	  return *this;
> -	}
> -
>  	constexpr _Tp&
>  	operator*() noexcept
>  	{ return this->_M_get(); }
> @@ -2886,7 +2843,7 @@ namespace views::__adaptor
>  	    if constexpr (forward_range<_Vp>)
>  	      return _M_current;
>  	    else
> -	      return *_M_parent->_M_current;
> +	      return _M_parent->_M_current;
>  	  }
>  
>  	  constexpr auto&
> @@ -2895,7 +2852,7 @@ namespace views::__adaptor
>  	    if constexpr (forward_range<_Vp>)
>  	      return _M_current;
>  	    else
> -	      return *_M_parent->_M_current;
> +	      return _M_parent->_M_current;
>  	  }
>  
>  	  _Parent* _M_parent = nullptr;
> @@ -3143,13 +3100,14 @@ namespace views::__adaptor
>        // XXX: _M_current is "present only if !forward_range<V>"
>        [[no_unique_address]]
>  	__detail::__maybe_present_t<!forward_range<_Vp>,
> -	  __detail::__non_propagating_cache<iterator_t<_Vp>>> _M_current;
> +				    iterator_t<_Vp>> _M_current;
>        _Vp _M_base = _Vp();
>  
>  
>      public:
>        split_view() requires (default_initializable<_Vp>
> -			     && default_initializable<_Pattern>)
> +			     && default_initializable<_Pattern>
> +			     && default_initializable<iterator_t<_Vp>>)
>  	= default;
>  
>        constexpr
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
> index fa6d4d56816..2078d442447 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
> @@ -101,6 +101,9 @@ test02()
>    __box<A<false>> x1(std::in_place, 0, 0);
>  }
>  
> +#if 0
> +// On the 11 branch, the partial specialization of __box admits only copyable types
> +// so this test doesn't apply.
>  constexpr bool
>  test03()
>  {
> @@ -142,3 +145,4 @@ test03()
>    return true;
>  }
>  static_assert(test03());
> +#endif
> -- 
> 2.35.1.102.g2b9c120970
> 
>
Jonathan Wakely Feb. 11, 2022, 7:47 p.m. UTC | #2 
On Fri, 11 Feb 2022, 17:11 Patrick Palka via Libstdc++, <
libstdc++@gcc.gnu.org> wrote:

> On Fri, 11 Feb 2022, Patrick Palka wrote:
>
> > In the P2325R3 backport r11-9555 the relaxation of the constraints on
> > the partial specialization of __box (which is semantically equivalent to
> > the primary template, only more space efficient) means some
> > specializations of __box will now use the partial specialization instead
> > of the primary template, which (IIUC) constitutes an ABI change
> unsuitable
> > for a release branch.  This patch reverts this constraint change, which
> > isn't needed for correctness anyway.
> >
> > Similarly the change to use __non_propagating_cache for the data member
> > split_view::_M_current (so that it's always default-initializable) also
> > constitutes an unsuitable ABI change.  This patch reverts this change
> > too, and instead further constrains split_view's default constructor to
> > require that we can default-initialize _M_current.
>
> Forgot to clarify that this is for the 11 branch, tested on
> x86_64-pc-linux-gnu.  Does this look reasonable?  I noticed these
> issues while backporting r11-9555 to the 10 branch, which doesn't have
> __non_propagating_cache or the partial specialization of __box.
>


Yes, thanks for spotting the problem. OK for gcc-11.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 03c6275801f..bf31e4be500 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -144,8 +144,7 @@  namespace ranges
     // std::optional.  It provides just the subset of the primary template's
     // API that we currently use.
     template<__boxable _Tp>
-      requires copyable<_Tp> || (is_nothrow_move_constructible_v<_Tp>
-				 && is_nothrow_copy_constructible_v<_Tp>)
+      requires copyable<_Tp>
       struct __box<_Tp>
       {
       private:
@@ -174,38 +173,6 @@  namespace ranges
 	  : _M_value(std::forward<_Args>(__args)...)
 	  { }
 
-	__box(const __box&) = default;
-	__box(__box&&) = default;
-	__box& operator=(const __box&) requires copyable<_Tp> = default;
-	__box& operator=(__box&&) requires copyable<_Tp> = default;
-
-	// When _Tp is nothrow_copy_constructible but not copy_assignable,
-	// copy assignment is implemented via destroy-then-copy-construct.
-	constexpr __box&
-	operator=(const __box& __that) noexcept
-	{
-	  static_assert(is_nothrow_copy_constructible_v<_Tp>);
-	  if (this != std::__addressof(__that))
-	    {
-	      _M_value.~_Tp();
-	      std::construct_at(std::__addressof(_M_value), *__that);
-	    }
-	  return *this;
-	}
-
-	// Likewise for move assignment.
-	constexpr __box&
-	operator=(__box&& __that) noexcept
-	{
-	  static_assert(is_nothrow_move_constructible_v<_Tp>);
-	  if (this != std::__addressof(__that))
-	    {
-	      _M_value.~_Tp();
-	      std::construct_at(std::__addressof(_M_value), std::move(*__that));
-	    }
-	  return *this;
-	}
-
 	constexpr bool
 	has_value() const noexcept
 	{ return true; };
@@ -1180,16 +1147,6 @@  namespace views::__adaptor
 	  return *this;
 	}
 
-	constexpr __non_propagating_cache&
-	operator=(_Tp __val)
-	{
-	  this->_M_reset();
-	  std::construct_at(std::__addressof(this->_M_payload._M_payload),
-			    std::in_place, std::move(__val));
-	  this->_M_payload._M_engaged = true;
-	  return *this;
-	}
-
 	constexpr _Tp&
 	operator*() noexcept
 	{ return this->_M_get(); }
@@ -2886,7 +2843,7 @@  namespace views::__adaptor
 	    if constexpr (forward_range<_Vp>)
 	      return _M_current;
 	    else
-	      return *_M_parent->_M_current;
+	      return _M_parent->_M_current;
 	  }
 
 	  constexpr auto&
@@ -2895,7 +2852,7 @@  namespace views::__adaptor
 	    if constexpr (forward_range<_Vp>)
 	      return _M_current;
 	    else
-	      return *_M_parent->_M_current;
+	      return _M_parent->_M_current;
 	  }
 
 	  _Parent* _M_parent = nullptr;
@@ -3143,13 +3100,14 @@  namespace views::__adaptor
       // XXX: _M_current is "present only if !forward_range<V>"
       [[no_unique_address]]
 	__detail::__maybe_present_t<!forward_range<_Vp>,
-	  __detail::__non_propagating_cache<iterator_t<_Vp>>> _M_current;
+				    iterator_t<_Vp>> _M_current;
       _Vp _M_base = _Vp();
 
 
     public:
       split_view() requires (default_initializable<_Vp>
-			     && default_initializable<_Pattern>)
+			     && default_initializable<_Pattern>
+			     && default_initializable<iterator_t<_Vp>>)
 	= default;
 
       constexpr
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
index fa6d4d56816..2078d442447 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/detail/copyable_box.cc
@@ -101,6 +101,9 @@  test02()
   __box<A<false>> x1(std::in_place, 0, 0);
 }
 
+#if 0
+// On the 11 branch, the partial specialization of __box admits only copyable types
+// so this test doesn't apply.
 constexpr bool
 test03()
 {
@@ -142,3 +145,4 @@  test03()
   return true;
 }
 static_assert(test03());
+#endif