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

[Jonathan Wakely]

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 <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?
>>
>>
>>
>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.