Extend usage of C++11 direct init in __debug::vector

Message ID b493e7b9-5755-7d3d-52da-ca47ae5fbfec@gmail.com
State New
Headers show
Series
  • Extend usage of C++11 direct init in __debug::vector
Related show

Commit Message

François Dumont Oct. 15, 2018, 5:23 a.m.
This patch extend usage of C++11 direct initialization in 
__debug::vector and makes some calls to operator - more consistent.

Note that I also rewrote following expression in erase method:

-      return begin() + (__first.base() - cbegin().base());
+      return { _Base::begin() + (__first.base() - _Base::cbegin()), this };

The latter version was building 2 safe iterators and incrementing 1 with 
the additional debug check inherent to such an operation whereas the new 
version just build 1 safe iterator with directly the expected offset.

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

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

Tested under Linux x86_64 Debug mode and committed.

François

Comments

Jonathan Wakely Oct. 15, 2018, 10:10 a.m. | #1
On 15/10/18 07:23 +0200, François Dumont wrote:
>This patch extend usage of C++11 direct initialization in 

>__debug::vector and makes some calls to operator - more consistent.

>

>Note that I also rewrote following expression in erase method:

>

>-      return begin() + (__first.base() - cbegin().base());

>+      return { _Base::begin() + (__first.base() - _Base::cbegin()), this };

>

>The latter version was building 2 safe iterators and incrementing 1 

>with the additional debug check inherent to such an operation whereas 

>the new version just build 1 safe iterator with directly the expected 

>offset.


Makes sense.

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

>

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

>    initialization.

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

>    (vector<>::emplace(const_iterator, _Args&&...)): Likewise and use

>    consistent iterator comparison.

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

>    (vector<>::insert(const_iterator, _InputIterator, _InputIterator)):

>    Likewise.

>    (vector<>::erase(const_iterator)): Likewise.

>    (vector<>::erase(const_iterator, const_iterator)): Likewise.

>

>Tested under Linux x86_64 Debug mode and committed.

>

>François

>


>@@ -542,7 +542,8 @@ namespace __debug

>       {

> 	__glibcxx_check_insert(__position);

> 	bool __realloc = this->_M_requires_reallocation(this->size() + 1);

>-	difference_type __offset = __position.base() - _Base::begin();

>+	difference_type __offset

>+	  = __position.base() - __position._M_get_sequence()->_M_base().begin();


What's the reason for this change?

Doesn't __glibcxx_check_insert(__position) already ensure that
__position is attached to *this, and so _Base::begin() returns the
same thing as __position._M_get_sequence()->_M_base().begin() ?

If they're equivalent, the original code seems more readable.
François Dumont Oct. 15, 2018, 8:45 p.m. | #2
On 10/15/2018 12:10 PM, Jonathan Wakely wrote:
> On 15/10/18 07:23 +0200, François Dumont wrote:

>> This patch extend usage of C++11 direct initialization in 

>> __debug::vector and makes some calls to operator - more consistent.

>>

>> Note that I also rewrote following expression in erase method:

>>

>> -      return begin() + (__first.base() - cbegin().base());

>> +      return { _Base::begin() + (__first.base() - _Base::cbegin()), 

>> this };

>>

>> The latter version was building 2 safe iterators and incrementing 1 

>> with the additional debug check inherent to such an operation whereas 

>> the new version just build 1 safe iterator with directly the expected 

>> offset.

>

> Makes sense.

>

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

>>

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

>>     initialization.

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

>>     (vector<>::emplace(const_iterator, _Args&&...)): Likewise and use

>>     consistent iterator comparison.

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

>>     (vector<>::insert(const_iterator, _InputIterator, _InputIterator)):

>>     Likewise.

>>     (vector<>::erase(const_iterator)): Likewise.

>>     (vector<>::erase(const_iterator, const_iterator)): Likewise.

>>

>> Tested under Linux x86_64 Debug mode and committed.

>>

>> François

>>

>

>> @@ -542,7 +542,8 @@ namespace __debug

>>       {

>>     __glibcxx_check_insert(__position);

>>     bool __realloc = this->_M_requires_reallocation(this->size() + 1);

>> -    difference_type __offset = __position.base() - _Base::begin();

>> +    difference_type __offset

>> +      = __position.base() - 

>> __position._M_get_sequence()->_M_base().begin();

>

> What's the reason for this change?

>

> Doesn't __glibcxx_check_insert(__position) already ensure that

> __position is attached to *this, and so _Base::begin() returns the

> same thing as __position._M_get_sequence()->_M_base().begin() ?

>

> If they're equivalent, the original code seems more readable.

>

>

>

This is the consistent iterator comparison part. Depending on C++ mode 
__position can be iterator or const_iterator.

As _M_get_sequence() return type depends on iterator type we always have 
iterator - iterator or const_iterator - const_iterator so no conversion.
Jonathan Wakely Oct. 16, 2018, 9:20 a.m. | #3
On 15/10/18 22:45 +0200, François Dumont wrote:
>On 10/15/2018 12:10 PM, Jonathan Wakely wrote:

>>On 15/10/18 07:23 +0200, François Dumont wrote:

>>>This patch extend usage of C++11 direct initialization in 

>>>__debug::vector and makes some calls to operator - more 

>>>consistent.

>>>

>>>Note that I also rewrote following expression in erase method:

>>>

>>>-      return begin() + (__first.base() - cbegin().base());

>>>+      return { _Base::begin() + (__first.base() - 

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

>>>

>>>The latter version was building 2 safe iterators and incrementing 

>>>1 with the additional debug check inherent to such an operation 

>>>whereas the new version just build 1 safe iterator with directly 

>>>the expected offset.

>>

>>Makes sense.

>>

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

>>>

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

>>>    initialization.

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

>>>    (vector<>::emplace(const_iterator, _Args&&...)): Likewise and use

>>>    consistent iterator comparison.

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

>>>    (vector<>::insert(const_iterator, _InputIterator, _InputIterator)):

>>>    Likewise.

>>>    (vector<>::erase(const_iterator)): Likewise.

>>>    (vector<>::erase(const_iterator, const_iterator)): Likewise.

>>>

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

>>>

>>>François

>>>

>>

>>>@@ -542,7 +542,8 @@ namespace __debug

>>>      {

>>>    __glibcxx_check_insert(__position);

>>>    bool __realloc = this->_M_requires_reallocation(this->size() + 1);

>>>-    difference_type __offset = __position.base() - _Base::begin();

>>>+    difference_type __offset

>>>+      = __position.base() - 

>>>__position._M_get_sequence()->_M_base().begin();

>>

>>What's the reason for this change?

>>

>>Doesn't __glibcxx_check_insert(__position) already ensure that

>>__position is attached to *this, and so _Base::begin() returns the

>>same thing as __position._M_get_sequence()->_M_base().begin() ?

>>

>>If they're equivalent, the original code seems more readable.

>>

>>

>>

>This is the consistent iterator comparison part. Depending on C++ mode 

>__position can be iterator or const_iterator.

>

>As _M_get_sequence() return type depends on iterator type we always 

>have iterator - iterator or const_iterator - const_iterator so no 

>conversion.



It used to work without a conversion. It only involves a conversion
because you removed this operator a few weeks ago:

-  template<typename _IteratorL, typename _IteratorR, typename _Sequence>
-    inline typename _Safe_iterator<_IteratorL, _Sequence,
-                       std::random_access_iterator_tag>::difference_type
-    operator-(const _Safe_iterator<_IteratorL, _Sequence,
-                                  std::random_access_iterator_tag>& __lhs,
-             const _Safe_iterator<_IteratorR, _Sequence,
-                                  std::random_access_iterator_tag>& __rhs)

So now there are extra conversions, and you're obfuscating the code to
avoid them.

Either the conversions are harmless and we don't need the operator, or
they're too expensive and we should keep the operator.
Jonathan Wakely Oct. 16, 2018, 9:24 a.m. | #4
On 16/10/18 10:20 +0100, Jonathan Wakely wrote:
>On 15/10/18 22:45 +0200, François Dumont wrote:

>>On 10/15/2018 12:10 PM, Jonathan Wakely wrote:

>>>On 15/10/18 07:23 +0200, François Dumont wrote:

>>>>This patch extend usage of C++11 direct initialization in 

>>>>__debug::vector and makes some calls to operator - more 

>>>>consistent.

>>>>

>>>>Note that I also rewrote following expression in erase method:

>>>>

>>>>-      return begin() + (__first.base() - cbegin().base());

>>>>+      return { _Base::begin() + (__first.base() - 

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

>>>>

>>>>The latter version was building 2 safe iterators and 

>>>>incrementing 1 with the additional debug check inherent to such 

>>>>an operation whereas the new version just build 1 safe iterator 

>>>>with directly the expected offset.

>>>

>>>Makes sense.

>>>

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

>>>>

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

>>>>    initialization.

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

>>>>    (vector<>::emplace(const_iterator, _Args&&...)): Likewise and use

>>>>    consistent iterator comparison.

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

>>>>    (vector<>::insert(const_iterator, _InputIterator, _InputIterator)):

>>>>    Likewise.

>>>>    (vector<>::erase(const_iterator)): Likewise.

>>>>    (vector<>::erase(const_iterator, const_iterator)): Likewise.

>>>>

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

>>>>

>>>>François

>>>>

>>>

>>>>@@ -542,7 +542,8 @@ namespace __debug

>>>>      {

>>>>    __glibcxx_check_insert(__position);

>>>>    bool __realloc = this->_M_requires_reallocation(this->size() + 1);

>>>>-    difference_type __offset = __position.base() - _Base::begin();

>>>>+    difference_type __offset

>>>>+      = __position.base() - 

>>>>__position._M_get_sequence()->_M_base().begin();

>>>

>>>What's the reason for this change?

>>>

>>>Doesn't __glibcxx_check_insert(__position) already ensure that

>>>__position is attached to *this, and so _Base::begin() returns the

>>>same thing as __position._M_get_sequence()->_M_base().begin() ?

>>>

>>>If they're equivalent, the original code seems more readable.

>>>

>>>

>>>

>>This is the consistent iterator comparison part. Depending on C++ 

>>mode __position can be iterator or const_iterator.

>>

>>As _M_get_sequence() return type depends on iterator type we always 

>>have iterator - iterator or const_iterator - const_iterator so no 

>>conversion.

>

>

>It used to work without a conversion. It only involves a conversion

>because you removed this operator a few weeks ago:

>

>-  template<typename _IteratorL, typename _IteratorR, typename _Sequence>

>-    inline typename _Safe_iterator<_IteratorL, _Sequence,

>-                       std::random_access_iterator_tag>::difference_type

>-    operator-(const _Safe_iterator<_IteratorL, _Sequence,

>-                                  std::random_access_iterator_tag>& __lhs,

>-             const _Safe_iterator<_IteratorR, _Sequence,

>-                                  std::random_access_iterator_tag>& __rhs)

>

>So now there are extra conversions, and you're obfuscating the code to

>avoid them.

>

>Either the conversions are harmless and we don't need the operator, or

>they're too expensive and we should keep the operator.


Oh, I got confused, the operation is on the underlying base
iterator type, not the safe iterators. So doesn't that use this
overload from <bits/stl_iterator.h> ?

  // _GLIBCXX_RESOLVE_LIB_DEFECTS
  // According to the resolution of DR179 not only the various comparison
  // operators but also operator- must accept mixed iterator/const_iterator
  // parameters.
  template<typename _IteratorL, typename _IteratorR, typename _Container>
#if __cplusplus >= 201103L
    // DR 685.
    inline auto
    operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
	      const __normal_iterator<_IteratorR, _Container>& __rhs) noexcept
    -> decltype(__lhs.base() - __rhs.base())
#else
    inline typename __normal_iterator<_IteratorL, _Container>::difference_type
    operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
	      const __normal_iterator<_IteratorR, _Container>& __rhs)
#endif
    { return __lhs.base() - __rhs.base(); }

Why would that involve any conversion?

Patch

diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index ff9f5f47c24..c11ddbb7048 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 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
@@ -521,7 +521,7 @@  namespace __debug
 	{
 	  __glibcxx_check_insert(__position);
 	  bool __realloc = this->_M_requires_reallocation(this->size() + 1);
-	  difference_type __offset = __position.base() - _Base::begin();
+	  difference_type __offset = __position.base() - _Base::cbegin();
 	  _Base_iterator __res = _Base::emplace(__position.base(),
 						std::forward<_Args>(__args)...);
 	  if (__realloc)
@@ -529,7 +529,7 @@  namespace __debug
 	  else
 	    this->_M_invalidate_after_nth(__offset);
 	  this->_M_update_guaranteed_capacity();
-	  return iterator(__res, this);
+	  return { __res, this };
 	}
 #endif
 
@@ -542,7 +542,8 @@  namespace __debug
       {
 	__glibcxx_check_insert(__position);
 	bool __realloc = this->_M_requires_reallocation(this->size() + 1);
-	difference_type __offset = __position.base() - _Base::begin();
+	difference_type __offset
+	  = __position.base() - __position._M_get_sequence()->_M_base().begin();
 	_Base_iterator __res = _Base::insert(__position.base(), __x);
 	if (__realloc)
 	  this->_M_invalidate_all();
@@ -577,7 +578,7 @@  namespace __debug
 	else
 	  this->_M_invalidate_after_nth(__offset);
 	this->_M_update_guaranteed_capacity();
-	return iterator(__res, this);
+	return { __res, this };
       }
 #else
       void
@@ -623,7 +624,7 @@  namespace __debug
 	  else
 	    this->_M_invalidate_after_nth(__offset);
 	  this->_M_update_guaranteed_capacity();
-	  return iterator(__res, this);
+	  return { __res, this };
 	}
 #else
       template<class _InputIterator>
@@ -661,7 +662,8 @@  namespace __debug
 #endif
       {
 	__glibcxx_check_erase(__position);
-	difference_type __offset = __position.base() - _Base::begin();
+	difference_type __offset
+	  = __position.base() - __position._M_get_sequence()->_M_base().begin();
 	_Base_iterator __res = _Base::erase(__position.base());
 	this->_M_invalidate_after_nth(__offset);
 	return iterator(__res, this);
@@ -680,7 +682,8 @@  namespace __debug
 
 	if (__first.base() != __last.base())
 	  {
-	    difference_type __offset = __first.base() - _Base::begin();
+	    difference_type __offset =
+	      __first.base() - __first._M_get_sequence()->_M_base().begin();
 	    _Base_iterator __res = _Base::erase(__first.base(),
 						__last.base());
 	    this->_M_invalidate_after_nth(__offset);
@@ -688,7 +691,7 @@  namespace __debug
 	  }
 	else
 #if __cplusplus >= 201103L
-	  return begin() + (__first.base() - cbegin().base());
+	  return { _Base::begin() + (__first.base() - _Base::cbegin()), this };
 #else
 	  return __first;
 #endif