* [PATCH] libstdc++: Back out some changes in P2325R3 backport [PR103904]
@ 2022-02-11 16:51 Patrick Palka
2022-02-11 17:09 ` Patrick Palka
0 siblings, 1 reply; 3+ messages in thread
From: Patrick Palka @ 2022-02-11 16:51 UTC (permalink / raw)
To: gcc-patches; +Cc: libstdc++, Patrick Palka
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(-)
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libstdc++: Back out some changes in P2325R3 backport [PR103904]
2022-02-11 16:51 [PATCH] libstdc++: Back out some changes in P2325R3 backport [PR103904] Patrick Palka
@ 2022-02-11 17:09 ` Patrick Palka
2022-02-11 19:47 ` Jonathan Wakely
0 siblings, 1 reply; 3+ messages in thread
From: Patrick Palka @ 2022-02-11 17:09 UTC (permalink / raw)
To: Patrick Palka; +Cc: gcc-patches, libstdc++
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libstdc++: Back out some changes in P2325R3 backport [PR103904]
2022-02-11 17:09 ` Patrick Palka
@ 2022-02-11 19:47 ` Jonathan Wakely
0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-02-11 19:47 UTC (permalink / raw)
To: Patrick Palka; +Cc: libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-11 19:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-11 16:51 [PATCH] libstdc++: Back out some changes in P2325R3 backport [PR103904] Patrick Palka
2022-02-11 17:09 ` Patrick Palka
2022-02-11 19:47 ` 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).