Thanks for the explanation, it is much clearer.
So here is a the more limited patch.
Tested under Linux x86_64.
Ok to commit ?
François
On 18/11/2015 13:27, Jonathan Wakely wrote:
> On 17/11/15 20:49 +0100, François Dumont wrote:
>> On 16/11/2015 11:29, Jonathan Wakely wrote:
>>> Not doing the checks is also an option. That minimizes the cost :-)
>>
>> This is controlled by a macro, users already have this option.
>
> True, but we're talking about maybe enabling these checks by default
> when building linux distributions.
>
>>>
>>> For the full debug mode we want to check everything we can, and accept
>>> that has a cost.
>>>
>>> For the lightweight one we need to evaluate the relative benefits. Is
>>> it worth adding checks for errors that only happen rarely? Does the
>>> benefit outweigh the cost?
>>>
>>> I'm still not convinced that's the case for the "valid range" checks.
>>> I'm willing to be convinced, but am not convinced yet.
>>
>> Ok so I will remove this check. And what about insert position check ? I
>> guess this one too so I will remove it too. Note that will only remain
>> checks on the most basic operations that is to say those on which the
>> check will have the biggest impact proportionally.
>
> Yes, that's a good point.
>
> But my unproven assumption is that it's more common to use operator[]
> incorrectly, rather than pass invalid iterators to range insert, which
> is a relatively "advanced" operation.
>
>
>> I would like we push the simplest version so that people can start
>> experimenting.
>>
>> I would also prefer concentrate on _GLIBCXX_DEBUG mode :-)
>>
>>>
>>>> It would be great to have it for gcc 6.0. I am working on the same
>>>> for other containers.
>>>
>>> Please don't do the valid range checks for std::deque, the checks are
>>> undefined for iterators into different containers and will not give a
>>> reliable answer.
>>
>> But debug mode is full of those checks, no ?
>
> They're supposed to be guarded by checks for _M_can_compare, if they
> aren't that's a regression. For debug mode we can tell whether two
> iterators are comparable, because they store a pointer back to their
> parent container. We can't check that in normal mode.
>
>
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 305d446..3b17fb4 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/assertions.h>
+
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -320,7 +322,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector(const vector& __x)
: _Base(__x.size(),
_Alloc_traits::_S_select_on_copy(__x._M_get_Tp_allocator()))
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -340,7 +343,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
/// Copy constructor with alternative allocator
vector(const vector& __x, const allocator_type& __a)
: _Base(__x.size(), __a)
- { this->_M_impl._M_finish =
+ {
+ this->_M_impl._M_finish =
std::__uninitialized_copy_a(__x.begin(), __x.end(),
this->_M_impl._M_start,
_M_get_Tp_allocator());
@@ -470,7 +474,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
vector&
operator=(initializer_list<value_type> __l)
{
- this->assign(__l.begin(), __l.end());
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
return *this;
}
#endif
@@ -532,7 +537,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
void
assign(initializer_list<value_type> __l)
- { this->assign(__l.begin(), __l.end()); }
+ {
+ this->_M_assign_aux(__l.begin(), __l.end(),
+ random_access_iterator_tag());
+ }
#endif
/// Get a copy of the memory allocation object.
@@ -694,7 +702,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, const value_type& __x)
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -714,7 +722,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
resize(size_type __new_size, value_type __x = value_type())
{
if (__new_size > size())
- insert(end(), __new_size - size(), __x);
+ _M_fill_insert(end(), __new_size - size(), __x);
else if (__new_size < size())
_M_erase_at_end(this->_M_impl._M_start + __new_size);
}
@@ -778,7 +786,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 +804,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 +864,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 +875,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 +886,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 +897,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 +975,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);
}
@@ -1030,7 +1057,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
*/
iterator
insert(const_iterator __position, initializer_list<value_type> __l)
- { return this->insert(__position, __l.begin(), __l.end()); }
+ {
+ auto __offset = __position - cbegin();
+ _M_range_insert(begin() + __offset, __l.begin(), __l.end(),
+ std::random_access_iterator_tag());
+ return begin() + __offset;
+ }
#endif
#if __cplusplus >= 201103L
@@ -1194,6 +1226,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());
@@ -1328,11 +1364,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
_M_assign_dispatch(_InputIterator __first, _InputIterator __last,
__false_type)
- {
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_assign_aux(__first, __last, _IterCategory());
- }
+ { _M_assign_aux(__first, __last, std::__iterator_category(__first)); }
// Called by the second assign_dispatch above
template<typename _InputIterator>
@@ -1351,7 +1383,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
void
_M_fill_assign(size_type __n, const value_type& __val);
-
// Internal insert functions follow.
// Called by the range insert to implement [23.1.1]/9
@@ -1370,9 +1401,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_insert_dispatch(iterator __pos, _InputIterator __first,
_InputIterator __last, __false_type)
{
- typedef typename std::iterator_traits<_InputIterator>::
- iterator_category _IterCategory;
- _M_range_insert(__pos, __first, __last, _IterCategory());
+ _M_range_insert(__pos, __first, __last,
+ std::__iterator_category(__first));
}
// Called by the second insert_dispatch above
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 34118a4..db2ca71 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -256,7 +256,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
if (__first == __last)
_M_erase_at_end(__cur);
else
- insert(end(), __first, __last);
+ _M_range_insert(end(), __first, __last,
+ std::__iterator_category(__first));
}
template<typename _Tp, typename _Alloc>
diff --git a/libstdc++-v3/include/debug/assertions.h b/libstdc++-v3/include/debug/assertions.h
index 9b9a48c..6e5b58f 100644
--- a/libstdc++-v3/include/debug/assertions.h
+++ b/libstdc++-v3/include/debug/assertions.h
@@ -33,10 +33,27 @@
# define _GLIBCXX_DEBUG_ASSERT(_Condition)
# define _GLIBCXX_DEBUG_PEDASSERT(_Condition)
-# define _GLIBCXX_DEBUG_ONLY(_Statement) ;
+# define _GLIBCXX_DEBUG_ONLY(_Statement)
+#endif
+
+#ifndef _GLIBCXX_ASSERTIONS
+# define __glibcxx_requires_non_empty_range(_First,_Last)
+# define __glibcxx_requires_nonempty()
+# define __glibcxx_requires_subscript(_N)
#else
+// Verify that [_First, _Last) forms a non-empty iterator range.
+# define __glibcxx_requires_non_empty_range(_First,_Last) \
+ __glibcxx_assert(__builtin_expect(_First != _Last, true))
+# define __glibcxx_requires_subscript(_N) \
+ __glibcxx_assert(__builtin_expect(_N < this->size(), true))
+// Verify that the container is nonempty
+# define __glibcxx_requires_nonempty() \
+ __glibcxx_assert(__builtin_expect(!this->empty(), true))
+#endif
+
+#ifdef _GLIBCXX_DEBUG
# define _GLIBCXX_DEBUG_ASSERT(_Condition) __glibcxx_assert(_Condition)
# ifdef _GLIBCXX_DEBUG_PEDANTIC
@@ -46,7 +63,6 @@
# endif
# define _GLIBCXX_DEBUG_ONLY(_Statement) _Statement
-
#endif
#endif // _GLIBCXX_DEBUG_ASSERTIONS
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index b5935fe..b7e325e 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -74,24 +74,11 @@ 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
-
#else
#include <debug/macros.h>
@@ -99,8 +86,6 @@ namespace __gnu_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) \
__glibcxx_check_sorted(_First,_Last)
# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred) \
@@ -121,11 +106,9 @@ namespace __gnu_debug
__glibcxx_check_heap(_First,_Last)
# 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) \
__glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_subscript(_N) __glibcxx_check_subscript(_N)
# define __glibcxx_requires_irreflexive(_First,_Last) \
__glibcxx_check_irreflexive(_First,_Last)
# define __glibcxx_requires_irreflexive2(_First,_Last) \
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index a2db00d..b628b06 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -138,6 +138,7 @@ namespace __gnu_debug
return __dist.first >= 0;
}
+ // Can't tell so assume it is fine.
return true;
}
