This is the mail archive of the 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: Extend usage of C++11 direct init in __debug::vector

On 10/16/2018 11:24 AM, Jonathan Wakely wrote:
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 <>

    * include/debug/vector (vector<>::cbegin()): Use C++11 direct
    (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)):
    (vector<>::erase(const_iterator)): Likewise.
    (vector<>::erase(const_iterator, const_iterator)): Likewise.

Tested under Linux x86_64 Debug mode and committed.


@@ -542,7 +542,8 @@ namespace __debug
    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> ?

 // 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())
   inline typename __normal_iterator<_IteratorL, _Container>::difference_type
   operator-(const __normal_iterator<_IteratorL, _Container>& __lhs,
          const __normal_iterator<_IteratorR, _Container>& __rhs)
   { return __lhs.base() - __rhs.base(); }

Why would that involve any conversion?

Yes, in this case there is no conversion. Even with _Safe_iterator<> there would have been no conversion cause I kept the const/non-const operators as conversion is expensive for those type of iterator.

So no, my attempt was just to make code simpler for the compiler, but not necessarily for humans.

I thought in using _Safe_iterator<>::_M_get_distance_from_begin but it implies useless debug checks. I'll see if I can find a nicer expression or if I just rollback those changes.

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