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

[Jonathan Wakely <jwakely at redhat dot com>]

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?