This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: vector lightweight debug mode


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

diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..b84e653 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -63,6 +63,8 @@
 #include <initializer_list>
 #endif
 
+#include <debug/debug.h>
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -778,7 +780,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       reference
       operator[](size_type __n) _GLIBCXX_NOEXCEPT
-      { return *(this->_M_impl._M_start + __n); }
+      {
+	__glibcxx_requires_subscript(__n);
+	return *(this->_M_impl._M_start + __n);
+      }
 
       /**
        *  @brief  Subscript access to the data contained in the %vector.
@@ -793,7 +798,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       const_reference
       operator[](size_type __n) const _GLIBCXX_NOEXCEPT
-      { return *(this->_M_impl._M_start + __n); }
+      {
+	__glibcxx_requires_subscript(__n);
+	return *(this->_M_impl._M_start + __n);
+      }
 
     protected:
       /// Safety check used only from at().
@@ -850,7 +858,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       reference
       front() _GLIBCXX_NOEXCEPT
-      { return *begin(); }
+      {
+	__glibcxx_requires_nonempty();
+	return *begin();
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the first
@@ -858,7 +869,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       const_reference
       front() const _GLIBCXX_NOEXCEPT
-      { return *begin(); }
+      {
+	__glibcxx_requires_nonempty();
+	return *begin();
+      }
 
       /**
        *  Returns a read/write reference to the data at the last
@@ -866,7 +880,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       reference
       back() _GLIBCXX_NOEXCEPT
-      { return *(end() - 1); }
+      {
+	__glibcxx_requires_nonempty();
+	return *(end() - 1);
+      }
       
       /**
        *  Returns a read-only (constant) reference to the data at the
@@ -874,7 +891,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       const_reference
       back() const _GLIBCXX_NOEXCEPT
-      { return *(end() - 1); }
+      {
+	__glibcxx_requires_nonempty();
+	return *(end() - 1);
+      }
 
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 464. Suggestion for new member functions in standard containers.
@@ -949,6 +969,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       void
       pop_back() _GLIBCXX_NOEXCEPT
       {
+	__glibcxx_requires_nonempty();
 	--this->_M_impl._M_finish;
 	_Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
       }
@@ -1051,6 +1072,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       iterator
       insert(const_iterator __position, size_type __n, const value_type& __x)
       {
+	__glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+			 && (__position < cend() || !(cend() < __position)));
 	difference_type __offset = __position - cbegin();
 	_M_fill_insert(begin() + __offset, __n, __x);
 	return begin() + __offset;
@@ -1071,7 +1094,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       void
       insert(iterator __position, size_type __n, const value_type& __x)
-      { _M_fill_insert(__position, __n, __x); }
+      {
+	__glibcxx_assert((begin() < __position || !(__position < begin()))
+			 && (__position < end() || !(end() < __position)));
+	_M_fill_insert(__position, __n, __x);
+      }
 #endif
 
 #if __cplusplus >= 201103L
@@ -1096,6 +1123,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
         insert(const_iterator __position, _InputIterator __first,
 	       _InputIterator __last)
         {
+	  __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+			   && (__position < cend() || !(cend() < __position)));
 	  difference_type __offset = __position - cbegin();
 	  _M_insert_dispatch(begin() + __offset,
 			     __first, __last, __false_type());
@@ -1121,6 +1150,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
         insert(iterator __position, _InputIterator __first,
 	       _InputIterator __last)
         {
+	  __glibcxx_assert((begin() < __position || !(__position < begin()))
+			   && (__position < end() || !(end() < __position)));
 	  // Check whether it's an integral type.  If so, it's not an iterator.
 	  typedef typename std::__is_integer<_InputIterator>::__type _Integral;
 	  _M_insert_dispatch(__position, __first, __last, _Integral());
@@ -1145,10 +1176,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       iterator
 #if __cplusplus >= 201103L
       erase(const_iterator __position)
-      { return _M_erase(begin() + (__position - cbegin())); }
+      {
+	__glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+			 && __position < cend());
+	return _M_erase(begin() + (__position - cbegin()));
+      }
 #else
       erase(iterator __position)
-      { return _M_erase(__position); }
+      {
+	__glibcxx_assert((begin() < __position || !(__position < begin()))
+			 && __position < end());
+	return _M_erase(__position);
+      }
 #endif
 
       /**
@@ -1173,13 +1212,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 #if __cplusplus >= 201103L
       erase(const_iterator __first, const_iterator __last)
       {
+	__glibcxx_assert(__first < __last || !(__last < __first));
+	__glibcxx_assert((cbegin() < __first || !(__first < cbegin()))
+			 && (__first < cend() || __first == __last));
+	__glibcxx_assert((cbegin() < __last || __first == __last)
+			 && (__last < cend() || !(cend() < __last)));
 	const auto __beg = begin();
 	const auto __cbeg = cbegin();
 	return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
       }
 #else
       erase(iterator __first, iterator __last)
-      { return _M_erase(__first, __last); }
+      {
+	__glibcxx_assert(__first < __last || !(__last < __first));
+	__glibcxx_assert((begin() < __first || !(__first < begin()))
+			 && (__first < end() || __first == __last));
+	__glibcxx_assert((begin() < __last || __first == __last)
+			 && (__last < end() || !(end() < __last)));
+	return _M_erase(__first, __last);
+      }
 #endif
 
       /**
@@ -1194,6 +1245,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       void
       swap(vector& __x) _GLIBCXX_NOEXCEPT
       {
+#if __cplusplus >= 201103L
+	__glibcxx_assert(_Alloc_traits::propagate_on_container_swap::value
+			 || _M_get_Tp_allocator() == __x._M_get_Tp_allocator());
+#endif
 	this->_M_impl._M_swap_data(__x._M_impl);
 	_Alloc_traits::_S_on_swap(_M_get_Tp_allocator(),
 	                          __x._M_get_Tp_allocator());
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..b00a620 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -107,10 +107,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
     vector<_Tp, _Alloc>::
 #if __cplusplus >= 201103L
     insert(const_iterator __position, const value_type& __x)
+    {
+      __glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+		       && (__position < cend() || !(cend() < __position)));
 #else
     insert(iterator __position, const value_type& __x)
-#endif
     {
+      __glibcxx_assert((begin() < __position || !(__position < begin()))
+		       && (__position < end() || !(end() < __position)));
+#endif
       const size_type __n = __position - begin();
       if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
 	  && __position == end())
@@ -301,6 +306,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       vector<_Tp, _Alloc>::
       emplace(const_iterator __position, _Args&&... __args)
       {
+	__glibcxx_assert((cbegin() < __position || !(__position < cbegin()))
+			 && (__position < cend() || !(cend() < __position)));
 	const size_type __n = __position - begin();
 	if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage
 	    && __position == end())
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..ea9c0c2 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,68 +74,66 @@ namespace __gnu_debug
 # define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
 # define __glibcxx_requires_string(_String)
 # define __glibcxx_requires_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N)
 # define __glibcxx_requires_irreflexive(_First,_Last)
 # define __glibcxx_requires_irreflexive2(_First,_Last)
 # define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
 # define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
 
-#ifdef _GLIBCXX_ASSERTIONS
-// Verify that [_First, _Last) forms a non-empty iterator range.
-# define __glibcxx_requires_non_empty_range(_First,_Last) \
-  __glibcxx_assert(_First != _Last)
-// Verify that the container is nonempty
-# define __glibcxx_requires_nonempty() \
-  __glibcxx_assert(! this->empty())
-#else
-# define __glibcxx_requires_non_empty_range(_First,_Last)
-# define __glibcxx_requires_nonempty()
 #endif
 
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
 #else
 
 # include <debug/macros.h>
 
-# define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
-# define __glibcxx_requires_valid_range(_First,_Last)	\
+# ifdef _GLIBCXX_DEBUG
+#  define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
+#  define __glibcxx_requires_valid_range(_First,_Last)	\
   __glibcxx_check_valid_range(_First,_Last)
-# define __glibcxx_requires_non_empty_range(_First,_Last)	\
-  __glibcxx_check_non_empty_range(_First,_Last)
-# define __glibcxx_requires_sorted(_First,_Last)	\
+#  define __glibcxx_requires_sorted(_First,_Last)	\
   __glibcxx_check_sorted(_First,_Last)
-# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)	\
+#  define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)	\
   __glibcxx_check_sorted_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_sorted_set(_First1,_Last1,_First2)	\
+#  define __glibcxx_requires_sorted_set(_First1,_Last1,_First2)	\
   __glibcxx_check_sorted_set(_First1,_Last1,_First2)
-# define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
+#  define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
   __glibcxx_check_sorted_set_pred(_First1,_Last1,_First2,_Pred)
-# define __glibcxx_requires_partitioned_lower(_First,_Last,_Value)	\
+#  define __glibcxx_requires_partitioned_lower(_First,_Last,_Value)	\
   __glibcxx_check_partitioned_lower(_First,_Last,_Value)
-# define __glibcxx_requires_partitioned_upper(_First,_Last,_Value)	\
+#  define __glibcxx_requires_partitioned_upper(_First,_Last,_Value)	\
   __glibcxx_check_partitioned_upper(_First,_Last,_Value)
-# define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
+#  define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
   __glibcxx_check_partitioned_lower_pred(_First,_Last,_Value,_Pred)
-# define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
+#  define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
   __glibcxx_check_partitioned_upper_pred(_First,_Last,_Value,_Pred)
-# define __glibcxx_requires_heap(_First,_Last)	\
+#  define __glibcxx_requires_heap(_First,_Last)	\
   __glibcxx_check_heap(_First,_Last)
-# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)	\
+#  define __glibcxx_requires_heap_pred(_First,_Last,_Pred)	\
   __glibcxx_check_heap_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
-# define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
-# define __glibcxx_requires_string_len(_String,_Len)	\
+#  define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
+#  define __glibcxx_requires_string_len(_String,_Len)	\
   __glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
-# define __glibcxx_requires_irreflexive(_First,_Last)	\
+#  define __glibcxx_requires_irreflexive(_First,_Last)	\
   __glibcxx_check_irreflexive(_First,_Last)
-# define __glibcxx_requires_irreflexive2(_First,_Last)	\
+#  define __glibcxx_requires_irreflexive2(_First,_Last)	\
   __glibcxx_check_irreflexive2(_First,_Last)
-# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)	\
+#  define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)	\
   __glibcxx_check_irreflexive_pred(_First,_Last,_Pred)
-# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)	\
+#  define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)	\
   __glibcxx_check_irreflexive_pred2(_First,_Last,_Pred)
 
-# include <debug/functions.h>
+#  include <debug/functions.h>
+#endif
+
+# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
+# define __glibcxx_requires_non_empty_range(_First,_Last)	\
+  __glibcxx_check_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty() __glibcxx_check_nonempty()
+
+# include <debug/formatter.h>
 
 #endif
 
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index fede4f0..7fccf28 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -406,49 +406,10 @@ namespace __debug
       }
 
       // element access:
-      reference
-      operator[](size_type __n) _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_subscript(__n);
-	return _M_base()[__n];
-      }
-
-      const_reference
-      operator[](size_type __n) const _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_subscript(__n);
-	return _M_base()[__n];
-      }
-
+      using _Base::operator[];
       using _Base::at;
-
-      reference
-      front() _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_nonempty();
-	return _Base::front();
-      }
-
-      const_reference
-      front() const _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_nonempty();
-	return _Base::front();
-      }
-
-      reference
-      back() _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_nonempty();
-	return _Base::back();
-      }
-
-      const_reference
-      back() const _GLIBCXX_NOEXCEPT
-      {
-	__glibcxx_check_nonempty();
-	return _Base::back();
-      }
+      using _Base::front;
+      using _Base::back;
 
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // DR 464. Suggestion for new member functions in standard containers.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]