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

[François Dumont]

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.