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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

François Dumont-2
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 <[hidden email]>
>>>>>
>>>>>     * 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?
>
>
>
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.

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Wakely-3
On 17/10/18 07:25 +0200, François Dumont wrote:

>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 <[hidden email]>
>>>>>>
>>>>>>    * 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?
>>
>>
>>
>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.

The original code `__position.base() - _Base::begin()` seems simpler
for human readers and for the compiler.

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Wakely-3
In reply to this post by François Dumont-2
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.
>
>2018-10-15  François Dumont  <[hidden email]>
>
>    * 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.


This test fails now:

#define _GLIBCXX_DEBUG
#include <list>

int main()
{
  std::list<int> l{11, 11, 11, 12, 12, 12, 13, 13, 13};
  auto m = std::next(l.begin(), 7);
  l.erase(l.begin(), m);
  if (l.size() != 2)
    throw 1;
}

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Wakely-3
On 17/10/18 16:54 +0100, 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.
>>
>>2018-10-15  François Dumont  <[hidden email]>
>>
>>    * 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.
>
>
>This test fails now:
>
>#define _GLIBCXX_DEBUG
>#include <list>
>
>int main()
>{
> std::list<int> l{11, 11, 11, 12, 12, 12, 13, 13, 13};
> auto m = std::next(l.begin(), 7);
> l.erase(l.begin(), m);
> if (l.size() != 2)
>   throw 1;
>}
>

Sorry, I posted this in the wrong thread. Obviously it's the
<debug/list> patch in r264911 that caused the problem.