On 14/09/2015 21:50, Jonathan Wakely wrote:
> On 14/09/15 20:27 +0200, François Dumont wrote:
>> Hi
>>
>>    Here is what I had in mind when talking about moving debug checks to
>> the lightweight debug checks.
>
> Ah yes, I hadn't thought about removing reundant checks from the
> __gnu_debug containers, but that makes sense.
>
>>    Sometimes the checks have been simply moved resulting in a simpler
>> debug vector implementation (front, back...). Sometimes I copy the
>> checks in a simpler form and kept the debug one too to make sure
>> execution of the debug code is fine.
>>
>>    I plan to do the same for other containers.
>>
>>    I still need to run tests, ok if tests are fine ?
>>
>> François
>>
>
>> diff --git a/libstdc++-v3/include/bits/stl_vector.h
>> b/libstdc++-v3/include/bits/stl_vector.h
>> index 305d446..89a9aec 100644
>> --- a/libstdc++-v3/include/bits/stl_vector.h
>> +++ b/libstdc++-v3/include/bits/stl_vector.h
>> @@ -449,6 +449,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>       vector&
>>       operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
>>       {
>> +    __glibcxx_assert(this != &__x);
>
> Please don't do this, it fails in valid programs. The standard needs
> to be fixed in this regard.

The debug mode check should be removed too then.

>
>>         constexpr bool __move_storage =
>>           _Alloc_traits::_S_propagate_on_move_assign()
>>           || _Alloc_traits::_S_always_equal();
>> @@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>        */
>>       reference
>>       operator[](size_type __n) _GLIBCXX_NOEXCEPT
>> -      { return *(this->_M_impl._M_start + __n); }
>> +      {
>> +    __glibcxx_assert(__n < size());
>> +    return *(this->_M_impl._M_start + __n);
>> +      }
>
> This could use __glibcxx_requires_subscript(__n), see the attached
> patch.

    I thought you didn't want to use anything from debug.h so I try to
do with only __glibcxx_assert coming from c++config. I think your patch
is missing include of debug.h.

    But I was going to propose to use _Error_formatter also in this
mode, I do not see any reason to not do so. The attached patch does just
that.

>
>>
>>       /**
>>        *  @brief  Subscript access to the data contained in the %vector.
>> @@ -793,7 +797,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>        */
>>       const_reference
>>       operator[](size_type __n) const _GLIBCXX_NOEXCEPT
>> -      { return *(this->_M_impl._M_start + __n); }
>> +      {
>> +    __glibcxx_assert(__n < size());
>> +    return *(this->_M_impl._M_start + __n);
>> +      }
>>
>>     protected:
>>       /// Safety check used only from at().
>> @@ -850,7 +857,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>        */
>>       reference
>>       front() _GLIBCXX_NOEXCEPT
>> -      { return *begin(); }
>> +      {
>> +    __glibcxx_assert(!empty());
>
> This is __glibcxx_requires_nonempty(), already defined for
> _GLIBCXX_ASSERTIONS.
>
>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>       iterator
>>       insert(const_iterator __position, size_type __n, const
>> value_type& __x)
>>       {
>> +    __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>     difference_type __offset = __position - cbegin();
>>     _M_fill_insert(begin() + __offset, __n, __x);
>>     return begin() + __offset;
>
> This is undefined behaviour, so I'd rather not add this check (I know
> it's on the google branch, but it's still undefined behaviour).

Why ? Because of the >= operator usage ? Is the attached patch better ?
< and == operators are well defined for a random access iterator, no ?

>
> We could do it with std::less, I suppose.
>
> I've attached the simplest checks I thought we should add for vector
> and deque.
>
>