This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] __debug::list use C++11 direct initialization


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);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]