diff mbox series

[1/3] libstdc++: Apply the move_iterator changes described in P1207R4

Message ID 20200204020724.217468-1-ppalka@redhat.com
State New
Headers show
Series [1/3] libstdc++: Apply the move_iterator changes described in P1207R4 | expand

Commit Message

Patrick Palka Feb. 4, 2020, 2:07 a.m. UTC 
These changes are needed for some of the tests in the constrained algorithm
patch, because they use move_iterator with an uncopyable output_iterator.  The
other changes described in the paper are already applied, it seems.

libstdc++-v3/ChangeLog:

	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move the
	iterator when initializing _M_current.
	(move_iterator::base): Split into two overloads differing in
	ref-qualifiers as in P1207R4.
---
 libstdc++-v3/include/bits/stl_iterator.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Jonathan Wakely Feb. 4, 2020, 10:41 a.m. UTC | #1 
On 03/02/20 21:07 -0500, Patrick Palka wrote:
>These changes are needed for some of the tests in the constrained algorithm
>patch, because they use move_iterator with an uncopyable output_iterator.  The
>other changes described in the paper are already applied, it seems.
>
>libstdc++-v3/ChangeLog:
>
>	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move the
>	iterator when initializing _M_current.
>	(move_iterator::base): Split into two overloads differing in
>	ref-qualifiers as in P1207R4.
>---
> libstdc++-v3/include/bits/stl_iterator.h | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
>index 784d200d22f..1a288a5c785 100644
>--- a/libstdc++-v3/include/bits/stl_iterator.h
>+++ b/libstdc++-v3/include/bits/stl_iterator.h
>@@ -1166,7 +1166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>       explicit _GLIBCXX17_CONSTEXPR
>       move_iterator(iterator_type __i)
>-      : _M_current(__i) { }
>+      : _M_current(std::move(__i)) { }
>
>       template<typename _Iter>
> 	_GLIBCXX17_CONSTEXPR
>@@ -1174,9 +1174,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	: _M_current(__i.base()) { }
>
>       _GLIBCXX17_CONSTEXPR iterator_type
>-      base() const
>+      base() const &
>+#if __cplusplus > 201703L && __cpp_lib_concepts
>+	requires copy_constructible<iterator_type>
>+#endif
>       { return _M_current; }
>
>+      _GLIBCXX17_CONSTEXPR iterator_type
>+      base() &&
>+      { return std::move(_M_current); }
>+

I think this change has to be restricted to C++20 and later, otherwise
a move_iterator in C++17 can change state so that its _M_current
becomes moved-from, which should not be possible before C++20.

So something like:

#if __cplusplus <= 201703L
       _GLIBCXX17_CONSTEXPR iterator_type
       base() const
       { return _M_current; }
#else
       constexpr iterator_type
       base() const &
#if __cpp_lib_concepts
	requires copy_constructible<iterator_type>
#endif
       { return _M_current; }

       constexpr iterator_type
       base() &&
       { return std::move(_M_current); }
#endif
Patrick Palka Feb. 4, 2020, 9:23 p.m. UTC | #2 
On Tue, 4 Feb 2020, Jonathan Wakely wrote:

> On 03/02/20 21:07 -0500, Patrick Palka wrote:
> > These changes are needed for some of the tests in the constrained algorithm
> > patch, because they use move_iterator with an uncopyable output_iterator.
> > The
> > other changes described in the paper are already applied, it seems.
> > 
> > libstdc++-v3/ChangeLog:
> > 
> > 	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move the
> > 	iterator when initializing _M_current.
> > 	(move_iterator::base): Split into two overloads differing in
> > 	ref-qualifiers as in P1207R4.
> > ---
> > libstdc++-v3/include/bits/stl_iterator.h | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> > b/libstdc++-v3/include/bits/stl_iterator.h
> > index 784d200d22f..1a288a5c785 100644
> > --- a/libstdc++-v3/include/bits/stl_iterator.h
> > +++ b/libstdc++-v3/include/bits/stl_iterator.h
> > @@ -1166,7 +1166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > 
> >       explicit _GLIBCXX17_CONSTEXPR
> >       move_iterator(iterator_type __i)
> > -      : _M_current(__i) { }
> > +      : _M_current(std::move(__i)) { }
> > 
> >       template<typename _Iter>
> > 	_GLIBCXX17_CONSTEXPR
> > @@ -1174,9 +1174,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > 	: _M_current(__i.base()) { }
> > 
> >       _GLIBCXX17_CONSTEXPR iterator_type
> > -      base() const
> > +      base() const &
> > +#if __cplusplus > 201703L && __cpp_lib_concepts
> > +	requires copy_constructible<iterator_type>
> > +#endif
> >       { return _M_current; }
> > 
> > +      _GLIBCXX17_CONSTEXPR iterator_type
> > +      base() &&
> > +      { return std::move(_M_current); }
> > +
> 
> I think this change has to be restricted to C++20 and later, otherwise
> a move_iterator in C++17 can change state so that its _M_current
> becomes moved-from, which should not be possible before C++20.
> 
> So something like:
> 
> #if __cplusplus <= 201703L
>       _GLIBCXX17_CONSTEXPR iterator_type
>       base() const
>       { return _M_current; }
> #else
>       constexpr iterator_type
>       base() const &
> #if __cpp_lib_concepts
> 	requires copy_constructible<iterator_type>
> #endif
>       { return _M_current; }
> 
>       constexpr iterator_type
>       base() &&
>       { return std::move(_M_current); }
> #endif
> 
> 

Thanks for the review, here is the updated patch.

libstdc++-v3/ChangeLog:

	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move __i
	when initializing _M_current.
	(move_iterator::base): Split into two overloads differing in
	ref-qualifiers as in P1207R4 for C++20.
---
 libstdc++-v3/include/bits/stl_iterator.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 784d200d22f..c200f7a9d14 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1166,16 +1166,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       explicit _GLIBCXX17_CONSTEXPR
       move_iterator(iterator_type __i)
-      : _M_current(__i) { }
+      : _M_current(std::move(__i)) { }
 
       template<typename _Iter>
 	_GLIBCXX17_CONSTEXPR
 	move_iterator(const move_iterator<_Iter>& __i)
 	: _M_current(__i.base()) { }
 
+#if __cplusplus <= 201703L
       _GLIBCXX17_CONSTEXPR iterator_type
       base() const
       { return _M_current; }
+#else
+      constexpr iterator_type
+      base() const &
+#if __cpp_lib_concepts
+	requires copy_constructible<iterator_type>
+#endif
+      { return _M_current; }
+
+      constexpr iterator_type
+      base() &&
+      { return std::move(_M_current); }
+#endif
 
       _GLIBCXX17_CONSTEXPR reference
       operator*() const
Jonathan Wakely Feb. 4, 2020, 9:49 p.m. UTC | #3 
On 04/02/20 16:23 -0500, Patrick Palka wrote:
>On Tue, 4 Feb 2020, Jonathan Wakely wrote:
>
>> On 03/02/20 21:07 -0500, Patrick Palka wrote:
>> > These changes are needed for some of the tests in the constrained algorithm
>> > patch, because they use move_iterator with an uncopyable output_iterator.
>> > The
>> > other changes described in the paper are already applied, it seems.
>> >
>> > libstdc++-v3/ChangeLog:
>> >
>> > 	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move the
>> > 	iterator when initializing _M_current.
>> > 	(move_iterator::base): Split into two overloads differing in
>> > 	ref-qualifiers as in P1207R4.
>> > ---
>> > libstdc++-v3/include/bits/stl_iterator.h | 11 +++++++++--
>> > 1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libstdc++-v3/include/bits/stl_iterator.h
>> > b/libstdc++-v3/include/bits/stl_iterator.h
>> > index 784d200d22f..1a288a5c785 100644
>> > --- a/libstdc++-v3/include/bits/stl_iterator.h
>> > +++ b/libstdc++-v3/include/bits/stl_iterator.h
>> > @@ -1166,7 +1166,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >
>> >       explicit _GLIBCXX17_CONSTEXPR
>> >       move_iterator(iterator_type __i)
>> > -      : _M_current(__i) { }
>> > +      : _M_current(std::move(__i)) { }
>> >
>> >       template<typename _Iter>
>> > 	_GLIBCXX17_CONSTEXPR
>> > @@ -1174,9 +1174,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > 	: _M_current(__i.base()) { }
>> >
>> >       _GLIBCXX17_CONSTEXPR iterator_type
>> > -      base() const
>> > +      base() const &
>> > +#if __cplusplus > 201703L && __cpp_lib_concepts
>> > +	requires copy_constructible<iterator_type>
>> > +#endif
>> >       { return _M_current; }
>> >
>> > +      _GLIBCXX17_CONSTEXPR iterator_type
>> > +      base() &&
>> > +      { return std::move(_M_current); }
>> > +
>>
>> I think this change has to be restricted to C++20 and later, otherwise
>> a move_iterator in C++17 can change state so that its _M_current
>> becomes moved-from, which should not be possible before C++20.
>>
>> So something like:
>>
>> #if __cplusplus <= 201703L
>>       _GLIBCXX17_CONSTEXPR iterator_type
>>       base() const
>>       { return _M_current; }
>> #else
>>       constexpr iterator_type
>>       base() const &
>> #if __cpp_lib_concepts
>> 	requires copy_constructible<iterator_type>
>> #endif
>>       { return _M_current; }
>>
>>       constexpr iterator_type
>>       base() &&
>>       { return std::move(_M_current); }
>> #endif
>>
>>
>
>Thanks for the review, here is the updated patch.

Thanks, this is OK for master (it's safe enough to change now, and it's
needed for some C++20-only changes that will also be safe to do now as
they don't touch exiting code).


>libstdc++-v3/ChangeLog:
>
>	* include/bits/stl_iterator.h (move_iterator::move_iterator): Move __i
>	when initializing _M_current.
>	(move_iterator::base): Split into two overloads differing in
>	ref-qualifiers as in P1207R4 for C++20.
>---
> libstdc++-v3/include/bits/stl_iterator.h | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
>index 784d200d22f..c200f7a9d14 100644
>--- a/libstdc++-v3/include/bits/stl_iterator.h
>+++ b/libstdc++-v3/include/bits/stl_iterator.h
>@@ -1166,16 +1166,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>       explicit _GLIBCXX17_CONSTEXPR
>       move_iterator(iterator_type __i)
>-      : _M_current(__i) { }
>+      : _M_current(std::move(__i)) { }
>
>       template<typename _Iter>
> 	_GLIBCXX17_CONSTEXPR
> 	move_iterator(const move_iterator<_Iter>& __i)
> 	: _M_current(__i.base()) { }
>
>+#if __cplusplus <= 201703L
>       _GLIBCXX17_CONSTEXPR iterator_type
>       base() const
>       { return _M_current; }
>+#else
>+      constexpr iterator_type
>+      base() const &
>+#if __cpp_lib_concepts
>+	requires copy_constructible<iterator_type>
>+#endif
>+      { return _M_current; }
>+
>+      constexpr iterator_type
>+      base() &&
>+      { return std::move(_M_current); }
>+#endif
>
>       _GLIBCXX17_CONSTEXPR reference
>       operator*() const
>-- 
>2.25.0.114.g5b0ca878e0
>
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 784d200d22f..1a288a5c785 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1166,7 +1166,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       explicit _GLIBCXX17_CONSTEXPR
       move_iterator(iterator_type __i)
-      : _M_current(__i) { }
+      : _M_current(std::move(__i)) { }
 
       template<typename _Iter>
 	_GLIBCXX17_CONSTEXPR
@@ -1174,9 +1174,16 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	: _M_current(__i.base()) { }
 
       _GLIBCXX17_CONSTEXPR iterator_type
-      base() const
+      base() const &
+#if __cplusplus > 201703L && __cpp_lib_concepts
+	requires copy_constructible<iterator_type>
+#endif
       { return _M_current; }
 
+      _GLIBCXX17_CONSTEXPR iterator_type
+      base() &&
+      { return std::move(_M_current); }
+
       _GLIBCXX17_CONSTEXPR reference
       operator*() const
       { return static_cast<reference>(*_M_current); }