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.
