On 27/06/2018 02:13, Jonathan Wakely wrote:
On 26/06/18 17:03 +0100, Jonathan Wakely wrote:
On 18/06/18 23:01 +0200, François Dumont wrote:
Hi
I abandon the idea of providing Debug algos, it would be too
much code to add and maintain. However I haven't quit on reducing
Debug mode performance impact.
So this patch make use of the existing std::__niter_base to get
rid of the debug layer around __gnu_debug::vector<>::iterator so
that __builtin_memmove replacement can take place.
As _Safe_iterator<> do not have a constructor taking a pointer I
change algos implementation so that we do not try to instantiate the
iterator type ourselves but rather rely on its operators + or -.
The small drawback is that for std::equal algo where we can't
use the __glibcxx_can_increment we need to keep the debug layer to
make sure we don't reach past-the-end iterator. So I had to remove
usage of __niter_base when in Debug mode, doing so it also disable
potential usage of __builtin_memcmp when calling std::equal on
std::__cxx1998::vector<> iterators. A rather rare situation I think.
We don't need to give up all checks in std::equal, we can do this:
@@ -1044,7 +1085,13 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
typename iterator_traits<_II1>::value_type,
typename iterator_traits<_II2>::value_type>)
__glibcxx_requires_valid_range(__first1, __last1);
-
+#ifdef _GLIBCXX_DEBUG
+ typedef typename iterator_traits<_II1>::iterator_category _Cat1;
+ typedef typename iterator_traits<_II2>::iterator_category _Cat2;
+ if (!__are_same<_Cat1, input_iterator_tag>::__value
+ && __are_same<_Cat2, random_access_iterator_tag>::__value)
+ __glibcxx_requires_can_increment_range(__first1, __last1,
__first2);
+#endif
return std::__equal_aux(std::__niter_base(__first1),
std::__niter_base(__last1),
std::__niter_base(__first2));
We don't give up any check. We only give up on using the
__builtin_memcmp when std::equal is being used while Debug mode is
active and std::equal is called directly with std::__cxx1998::vector.
Moreover this check is wrong and will introduce regression when running
testsuite in Debug mode (this is why I know). It will assert in the
following case:
vector<int> v1 { 0, 0, 0 };
vector<int> v2 { 0, 1 };
equal(v1.begin(), v1.end(), v2.begin());
Note that I don't know how to test that __builtin_memmove has
been indeed used. So I've been through some debug sessions to check
that.
The attached patch (not fully tested) seems to be a much simpler way
to achieve the same thing. Instead of modifying all the helper
structs, just define a new function to re-wrap the result into the
desired iterator type.
diff --git a/libstdc++-v3/include/debug/stl_iterator.h
b/libstdc++-v3/include/debug/stl_iterator.h
index a6a2a76..eca7203 100644
--- a/libstdc++-v3/include/debug/stl_iterator.h
+++ b/libstdc++-v3/include/debug/stl_iterator.h
@@ -120,4 +120,19 @@ namespace __gnu_debug
#endif
}
+#if __cplusplus >= 201103L
+namespace std
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+
+template<typename _Iterator, typename _Container, typename _Sequence>
+ _Iterator
+ __niter_base(const __gnu_debug::_Safe_iterator<
+ __gnu_cxx::__normal_iterator<_Iterator, _Container>,
+ _Sequence>&);
+
+_GLIBCXX_END_NAMESPACE_VERSION
+}
+#endif
Why is this overload only defined for C++11 and later? I defined it
unconditionally in the attached patch.
What do you think?
Here's a complete patch that passes all tests in normal mode and
causes no regressions in debug mode (we already have some debug test
failing).
I wondered whether we need another overload of __wrap_iter for
handling move_iterator, but I think the first overload works OK.
I don't think we need it neither. Algos that handle move iterator are
already doing it.
François
