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


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?



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