__debug::list use C++11 direct initialization

Message ID 926a61ca-121a-39c9-72fa-5327dd9ddc6b@gmail.com
State New
Headers show
Series
  • __debug::list use C++11 direct initialization
Related show

Commit Message

François Dumont Oct. 9, 2018, 5:11 a.m.
Here is the communication for my yesterday's patch which I thought svn 
had failed to commit (I had to interrupt it).

Similarly to what I've done for associative containers here is a cleanup 
of the std::__debug::list implementation leveraging more on C++11 direct 
initialization.

I also made sure we use consistent comparison between 
iterator/const_iterator in erase and emplace methods.

2018-10-08  François Dumont <fdumont@gcc.gnu.org>

     * include/debug/list (list<>::cbegin()): Use C++11 direct
     initialization.
     (list<>::cend()): Likewise.
     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
     (list<>::insert(const_iterator, initializer_list<>)): Likewise.
     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
     (list<>::erase(const_iterator, const_iterator)): Ensure consistent
     iterator comparisons.
     (list<>::splice(const_iterator, list&&, const_iterator,
     const_iterator)): Likewise.

Tested under Linux x86_64 Debug mode and committed.

François

Comments

Jonathan Wakely Oct. 15, 2018, 10:07 a.m. | #1
On 09/10/18 07:11 +0200, François Dumont wrote:
>Here is the communication for my yesterday's patch which I thought svn 

>had failed to commit (I had to interrupt it).

>

>Similarly to what I've done for associative containers here is a 

>cleanup of the std::__debug::list implementation leveraging more on 

>C++11 direct initialization.

>

>I also made sure we use consistent comparison between 

>iterator/const_iterator in erase and emplace methods.

>

>2018-10-08  François Dumont <fdumont@gcc.gnu.org>

>

>    * include/debug/list (list<>::cbegin()): Use C++11 direct

>    initialization.

>    (list<>::cend()): Likewise.

>    (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.

>    (list<>::insert(const_iterator, initializer_list<>)): Likewise.

>    (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.

>    (list<>::erase(const_iterator, const_iterator)): Ensure consistent

>    iterator comparisons.

>    (list<>::splice(const_iterator, list&&, const_iterator,

>    const_iterator)): Likewise.

>

>Tested under Linux x86_64 Debug mode and committed.

>

>François

>


>diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list

>index 8add1d596e0..879e1177497 100644

>--- a/libstdc++-v3/include/debug/list

>+++ b/libstdc++-v3/include/debug/list

>@@ -244,11 +244,11 @@ namespace __debug

> #if __cplusplus >= 201103L

>       const_iterator

>       cbegin() const noexcept

>-      { return const_iterator(_Base::begin(), this); }

>+      { return { _Base::begin(), this }; }

> 

>       const_iterator

>       cend() const noexcept

>-      { return const_iterator(_Base::end(), this); }

>+      { return { _Base::end(), this }; }


For functions like emplace (which are C++11-only) and for forward_list
(also C++11-only) using this syntax makes it clearer.

But for these functions it just makes cbegin() and cend() look
different to the C++98 begin() and end() functions, for no obvious
benefit.

Simply using { return end(); } would have been another option.
François Dumont Oct. 16, 2018, 5:06 a.m. | #2
On 10/15/2018 12:07 PM, Jonathan Wakely wrote:
> On 09/10/18 07:11 +0200, François Dumont wrote:

>> Here is the communication for my yesterday's patch which I thought 

>> svn had failed to commit (I had to interrupt it).

>>

>> Similarly to what I've done for associative containers here is a 

>> cleanup of the std::__debug::list implementation leveraging more on 

>> C++11 direct initialization.

>>

>> I also made sure we use consistent comparison between 

>> iterator/const_iterator in erase and emplace methods.

>>

>> 2018-10-08  François Dumont <fdumont@gcc.gnu.org>

>>

>>     * include/debug/list (list<>::cbegin()): Use C++11 direct

>>     initialization.

>>     (list<>::cend()): Likewise.

>>     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.

>>     (list<>::insert(const_iterator, initializer_list<>)): Likewise.

>>     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.

>>     (list<>::erase(const_iterator, const_iterator)): Ensure consistent

>>     iterator comparisons.

>>     (list<>::splice(const_iterator, list&&, const_iterator,

>>     const_iterator)): Likewise.

>>

>> Tested under Linux x86_64 Debug mode and committed.

>>

>> François

>>

>

>> diff --git a/libstdc++-v3/include/debug/list 

>> b/libstdc++-v3/include/debug/list

>> index 8add1d596e0..879e1177497 100644

>> --- a/libstdc++-v3/include/debug/list

>> +++ b/libstdc++-v3/include/debug/list

>> @@ -244,11 +244,11 @@ namespace __debug

>> #if __cplusplus >= 201103L

>>       const_iterator

>>       cbegin() const noexcept

>> -      { return const_iterator(_Base::begin(), this); }

>> +      { return { _Base::begin(), this }; }

>>

>>       const_iterator

>>       cend() const noexcept

>> -      { return const_iterator(_Base::end(), this); }

>> +      { return { _Base::end(), this }; }

>

> For functions like emplace (which are C++11-only) and for forward_list

> (also C++11-only) using this syntax makes it clearer.

>

> But for these functions it just makes cbegin() and cend() look

> different to the C++98 begin() and end() functions, for no obvious

> benefit.

>

> Simply using { return end(); } would have been another option.

>

Personnaly I hesitated in writting:

{ return { _Base::cbegin(), this }; }

cause I prefer when you see clearly that Debug implem forward calls to 
the Normal implem.

I thought that using C++11 direct init was more likely to have gcc elide 
the copy constructor so I used it everywhere possible.
Jonathan Wakely Oct. 16, 2018, 8:15 a.m. | #3
On 16/10/18 07:06 +0200, François Dumont wrote:
>On 10/15/2018 12:07 PM, Jonathan Wakely wrote:

>>On 09/10/18 07:11 +0200, François Dumont wrote:

>>>Here is the communication for my yesterday's patch which I thought 

>>>svn had failed to commit (I had to interrupt it).

>>>

>>>Similarly to what I've done for associative containers here is a 

>>>cleanup of the std::__debug::list implementation leveraging more 

>>>on C++11 direct initialization.

>>>

>>>I also made sure we use consistent comparison between 

>>>iterator/const_iterator in erase and emplace methods.

>>>

>>>2018-10-08  François Dumont <fdumont@gcc.gnu.org>

>>>

>>>    * include/debug/list (list<>::cbegin()): Use C++11 direct

>>>    initialization.

>>>    (list<>::cend()): Likewise.

>>>    (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.

>>>    (list<>::insert(const_iterator, initializer_list<>)): Likewise.

>>>    (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.

>>>    (list<>::erase(const_iterator, const_iterator)): Ensure consistent

>>>    iterator comparisons.

>>>    (list<>::splice(const_iterator, list&&, const_iterator,

>>>    const_iterator)): Likewise.

>>>

>>>Tested under Linux x86_64 Debug mode and committed.

>>>

>>>François

>>>

>>

>>>diff --git a/libstdc++-v3/include/debug/list 

>>>b/libstdc++-v3/include/debug/list

>>>index 8add1d596e0..879e1177497 100644

>>>--- a/libstdc++-v3/include/debug/list

>>>+++ b/libstdc++-v3/include/debug/list

>>>@@ -244,11 +244,11 @@ namespace __debug

>>>#if __cplusplus >= 201103L

>>>      const_iterator

>>>      cbegin() const noexcept

>>>-      { return const_iterator(_Base::begin(), this); }

>>>+      { return { _Base::begin(), this }; }

>>>

>>>      const_iterator

>>>      cend() const noexcept

>>>-      { return const_iterator(_Base::end(), this); }

>>>+      { return { _Base::end(), this }; }

>>

>>For functions like emplace (which are C++11-only) and for forward_list

>>(also C++11-only) using this syntax makes it clearer.

>>

>>But for these functions it just makes cbegin() and cend() look

>>different to the C++98 begin() and end() functions, for no obvious

>>benefit.

>>

>>Simply using { return end(); } would have been another option.

>>

>Personnaly I hesitated in writting:

>

>{ return { _Base::cbegin(), this }; }

>

>cause I prefer when you see clearly that Debug implem forward calls to 

>the Normal implem.

>

>I thought that using C++11 direct init was more likely to have gcc 

>elide the copy constructor so I used it everywhere possible.


It should make no difference to elision at all.
François Dumont Oct. 18, 2018, 5:38 a.m. | #4
I've just reverted the controversial changes.

2018-10-18  François Dumont  <fdumont@gcc.gnu.org>

     Partial revert.
     2018-10-08  François Dumont  <fdumont@gcc.gnu.org>

     * include/debug/list (list<>::cbegin()): Use C++11 direct
     initialization.
     (list<>::cend()): Likewise.
     (list<>::erase(const_iterator, const_iterator)): Ensure consistent
     iterator comparisons.
     (list<>::splice(const_iterator, list&&, const_iterator,
     const_iterator)): Likewise.

     Partial revert.
     2018-10-15  François Dumont  <fdumont@gcc.gnu.org>

     * include/debug/vector (vector<>::cbegin()): Use C++11 direct
     initialization.
     (vector<>::cend()): Likewise.
     (vector<>::insert(const_iterator, const _Tp&)): Use consistent
     iterator comparison.
     (vector<>::erase(const_iterator)): Likewise.
     (vector<>::erase(const_iterator, const_iterator)): Likewise.

François

On 10/17/2018 06:10 PM, Jonathan Wakely wrote:
> On 17/10/18 17:03 +0100, Jonathan Wakely wrote:

>> On 09/10/18 07:11 +0200, François Dumont wrote:

>>> Here is the communication for my yesterday's patch which I thought 

>>> svn had failed to commit (I had to interrupt it).

>>>

>>> Similarly to what I've done for associative containers here is a 

>>> cleanup of the std::__debug::list implementation leveraging more on 

>>> C++11 direct initialization.

>>>

>>> I also made sure we use consistent comparison between 

>>> iterator/const_iterator in erase and emplace methods.

>>>

>>> 2018-10-08  François Dumont <fdumont@gcc.gnu.org>

>>>

>>>     * include/debug/list (list<>::cbegin()): Use C++11 direct

>>>     initialization.

>>>     (list<>::cend()): Likewise.

>>>     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.

>>>     (list<>::insert(const_iterator, initializer_list<>)): Likewise.

>>>     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.

>>>     (list<>::erase(const_iterator, const_iterator)): Ensure consistent

>>>     iterator comparisons.

>>>     (list<>::splice(const_iterator, list&&, const_iterator,

>>>     const_iterator)): Likewise.

>>>

>>> Tested under Linux x86_64 Debug mode and committed.

>>>

>>> François

>>>

>>

>>> diff --git a/libstdc++-v3/include/debug/list 

>>> b/libstdc++-v3/include/debug/list

>>> index 8add1d596e0..879e1177497 100644

>>> --- a/libstdc++-v3/include/debug/list

>>> +++ b/libstdc++-v3/include/debug/list

>>> @@ -521,15 +521,17 @@ namespace __debug

>>>     // _GLIBCXX_RESOLVE_LIB_DEFECTS

>>>     // 151. can't currently clear() empty container

>>>     __glibcxx_check_erase_range(__first, __last);

>>> -    for (_Base_const_iterator __victim = __first.base();

>>> +    for (__decltype(__first.base()) __victim = __first.base();

>>

>> This change causes the regression.

>>

>> I think the problem is that the decltype is a reference, so every time

>> the loop does ++__victim it changes the iterator stored in __first.

>

>

> This would work:

>

>        typedef typename __decltype(__first)::iterator_type _Base_iter;

>     for (_Base_iter __victim = __first.base();

>          __victim != __last.base(); ++__victim)

>

>

> Or simply:

>

> #if __cplusplus >= 201103L

>        typedef _Base_const_iterator _Base_iter;

> #else

>        typedef _Base_iterator _Base_iter;

> #endif

>     for (_Base_iter __victim = __first.base();

>          __victim != __last.base(); ++__victim)

>

>

> Or just restore the original code which worked fine!

>

> .

>
diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
index 879e1177497..aab146d058b 100644
--- a/libstdc++-v3/include/debug/list
+++ b/libstdc++-v3/include/debug/list
@@ -244,11 +244,11 @@ namespace __debug
 #if __cplusplus >= 201103L
       const_iterator
       cbegin() const noexcept
-      { return { _Base::begin(), this }; }
+      { return const_iterator(_Base::begin(), this); }
 
       const_iterator
       cend() const noexcept
-      { return { _Base::end(), this }; }
+      { return const_iterator(_Base::end(), this); }
 
       const_reverse_iterator
       crbegin() const noexcept
@@ -521,14 +521,13 @@ namespace __debug
 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
 	// 151. can't currently clear() empty container
 	__glibcxx_check_erase_range(__first, __last);
-	for (__decltype(__first.base()) __victim = __first.base();
+	for (_Base_const_iterator __victim = __first.base();
 	     __victim != __last.base(); ++__victim)
 	  {
-	    _GLIBCXX_DEBUG_VERIFY(
-		__victim != __first._M_get_sequence()->_M_base().end(),
-		_M_message(__gnu_debug::__msg_valid_range)
-		._M_iterator(__first, "position")
-		._M_iterator(__last, "last"));
+	    _GLIBCXX_DEBUG_VERIFY(__victim != _Base::end(),
+				  _M_message(__gnu_debug::__msg_valid_range)
+				  ._M_iterator(__first, "position")
+				  ._M_iterator(__last, "last"));
 	    this->_M_invalidate_if(_Equal(__victim));
 	  }
 
@@ -622,14 +621,13 @@ namespace __debug
 	// We used to perform the splice_alloc check:  not anymore, redundant
 	// after implementing the relevant bits of N1599.
 
-	for (__decltype(__first.base()) __tmp = __first.base();
+	for (_Base_const_iterator __tmp = __first.base();
 	     __tmp != __last.base(); ++__tmp)
 	  {
-	    _GLIBCXX_DEBUG_VERIFY(
-		__tmp != __first._M_get_sequence()->_M_base().end(),
-		_M_message(__gnu_debug::__msg_valid_range)
-		._M_iterator(__first, "first")
-		._M_iterator(__last, "last"));
+	    _GLIBCXX_DEBUG_VERIFY(__tmp != _Base::end(),
+				  _M_message(__gnu_debug::__msg_valid_range)
+				  ._M_iterator(__first, "first")
+				  ._M_iterator(__last, "last"));
 	    _GLIBCXX_DEBUG_VERIFY(std::__addressof(__x) != this
 				  || __tmp != __position.base(),
 				_M_message(__gnu_debug::__msg_splice_overlap)
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index c11ddbb7048..0b712ba24e9 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -328,11 +328,11 @@ namespace __debug
 #if __cplusplus >= 201103L
       const_iterator
       cbegin() const noexcept
-      { return { _Base::begin(), this }; }
+      { return const_iterator(_Base::begin(), this); }
 
       const_iterator
       cend() const noexcept
-      { return { _Base::end(), this }; }
+      { return const_iterator(_Base::end(), this); }
 
       const_reverse_iterator
       crbegin() const noexcept
@@ -542,8 +542,7 @@ namespace __debug
       {
 	__glibcxx_check_insert(__position);
 	bool __realloc = this->_M_requires_reallocation(this->size() + 1);
-	difference_type __offset
-	  = __position.base() - __position._M_get_sequence()->_M_base().begin();
+	difference_type __offset = __position.base() - _Base::begin();
 	_Base_iterator __res = _Base::insert(__position.base(), __x);
 	if (__realloc)
 	  this->_M_invalidate_all();
@@ -662,8 +661,7 @@ namespace __debug
 #endif
       {
 	__glibcxx_check_erase(__position);
-	difference_type __offset
-	  = __position.base() - __position._M_get_sequence()->_M_base().begin();
+	difference_type __offset = __position.base() - _Base::begin();
 	_Base_iterator __res = _Base::erase(__position.base());
 	this->_M_invalidate_after_nth(__offset);
 	return iterator(__res, this);
@@ -682,8 +680,7 @@ namespace __debug
 
 	if (__first.base() != __last.base())
 	  {
-	    difference_type __offset =
-	      __first.base() - __first._M_get_sequence()->_M_base().begin();
+	    difference_type __offset = __first.base() - _Base::begin();
 	    _Base_iterator __res = _Base::erase(__first.base(),
 						__last.base());
 	    this->_M_invalidate_after_nth(__offset);

Patch

diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
index 8add1d596e0..879e1177497 100644
--- a/libstdc++-v3/include/debug/list
+++ b/libstdc++-v3/include/debug/list
@@ -244,11 +244,11 @@  namespace __debug
 #if __cplusplus >= 201103L
       const_iterator
       cbegin() const noexcept
-      { return const_iterator(_Base::begin(), this); }
+      { return { _Base::begin(), this }; }
 
       const_iterator
       cend() const noexcept
-      { return const_iterator(_Base::end(), this); }
+      { return { _Base::end(), this }; }
 
       const_reverse_iterator
       crbegin() const noexcept
@@ -405,8 +405,8 @@  namespace __debug
 	emplace(const_iterator __position, _Args&&... __args)
 	{
 	  __glibcxx_check_insert(__position);
-	  return iterator(_Base::emplace(__position.base(),
-					std::forward<_Args>(__args)...), this);
+	  return  { _Base::emplace(__position.base(),
+				   std::forward<_Args>(__args)...), this };
 	}
 #endif
 
@@ -430,7 +430,7 @@  namespace __debug
       insert(const_iterator __p, initializer_list<value_type> __l)
       {
 	__glibcxx_check_insert(__p);
-	return iterator(_Base::insert(__p.base(), __l), this);
+	return { _Base::insert(__p.base(), __l), this };
       }
 #endif
 
@@ -439,7 +439,7 @@  namespace __debug
       insert(const_iterator __position, size_type __n, const _Tp& __x)
       {
 	__glibcxx_check_insert(__position);
-	return iterator(_Base::insert(__position.base(), __n, __x), this);
+	return { _Base::insert(__position.base(), __n, __x), this };
       }
 #else
       void
@@ -465,7 +465,7 @@  namespace __debug
 		_Base::insert(__position.base(),
 			      __gnu_debug::__unsafe(__first),
 			      __gnu_debug::__unsafe(__last)),
-		  this
+		this
 	      };
 	  else
 	    return { _Base::insert(__position.base(), __first, __last), this };
@@ -521,15 +521,17 @@  namespace __debug
 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
 	// 151. can't currently clear() empty container
 	__glibcxx_check_erase_range(__first, __last);
-	for (_Base_const_iterator __victim = __first.base();
+	for (__decltype(__first.base()) __victim = __first.base();
 	     __victim != __last.base(); ++__victim)
 	  {
-	    _GLIBCXX_DEBUG_VERIFY(__victim != _Base::end(),
-				  _M_message(__gnu_debug::__msg_valid_range)
-				  ._M_iterator(__first, "position")
-				  ._M_iterator(__last, "last"));
+	    _GLIBCXX_DEBUG_VERIFY(
+		__victim != __first._M_get_sequence()->_M_base().end(),
+		_M_message(__gnu_debug::__msg_valid_range)
+		._M_iterator(__first, "position")
+		._M_iterator(__last, "last"));
 	    this->_M_invalidate_if(_Equal(__victim));
 	  }
+
 	return iterator(_Base::erase(__first.base(), __last.base()), this);
       }
 
@@ -586,7 +588,7 @@  namespace __debug
 			      ._M_iterator(__i, "__i"));
 	_GLIBCXX_DEBUG_VERIFY(__i._M_attached_to(std::__addressof(__x)),
 			      _M_message(__gnu_debug::__msg_splice_other)
-			     ._M_iterator(__i, "__i")._M_sequence(__x, "__x"));
+			      ._M_iterator(__i, "__i")._M_sequence(__x, "__x"));
 
 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
 	// 250. splicing invalidates iterators
@@ -620,19 +622,21 @@  namespace __debug
 	// We used to perform the splice_alloc check:  not anymore, redundant
 	// after implementing the relevant bits of N1599.
 
-	for (_Base_const_iterator __tmp = __first.base();
+	for (__decltype(__first.base()) __tmp = __first.base();
 	     __tmp != __last.base(); ++__tmp)
 	  {
-	    _GLIBCXX_DEBUG_VERIFY(__tmp != _Base::end(),
-				  _M_message(__gnu_debug::__msg_valid_range)
-				  ._M_iterator(__first, "first")
-				  ._M_iterator(__last, "last"));
+	    _GLIBCXX_DEBUG_VERIFY(
+		__tmp != __first._M_get_sequence()->_M_base().end(),
+		_M_message(__gnu_debug::__msg_valid_range)
+		._M_iterator(__first, "first")
+		._M_iterator(__last, "last"));
 	    _GLIBCXX_DEBUG_VERIFY(std::__addressof(__x) != this
 				  || __tmp != __position.base(),
 				_M_message(__gnu_debug::__msg_splice_overlap)
 				  ._M_iterator(__tmp, "position")
 				  ._M_iterator(__first, "first")
 				  ._M_iterator(__last, "last"));
+
 	    // _GLIBCXX_RESOLVE_LIB_DEFECTS
 	    // 250. splicing invalidates iterators
 	    this->_M_transfer_from_if(__x, _Equal(__tmp));