Re: vector lightweight debug mode

[Jonathan Wakely <jwakely at redhat dot com>]

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.

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

>       /**
>        *  @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).

We could do it with std::less, I suppose.

I've attached the simplest checks I thought we should add for vector
and deque.

Attachment: debug.patch
Description: Text document