Re: vector lightweight debug mode

[Jonathan Wakely]

On 16/09/15 21:37 +0200, François Dumont wrote:

On 14/09/2015 21:50, Jonathan Wakely wrote:

On 14/09/15 20:27 +0200, François Dumont wrote:

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.

Yes.

        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.

That pulls in extra dependencies on I/O and fprintf and things, which
can cause code size to increase. Is it really worth it?

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

No, because it is undefined to compare iterators that belong to
different containers, or to compare pointers that point to different
arrays.