| Message ID | b493e7b9-5755-7d3d-52da-ca47ae5fbfec@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Extend usage of C++11 direct init in __debug::vector | expand |
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.
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.
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.
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?
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
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