On 27/06/18 21:25 +0200, François Dumont wrote:
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 do for my version of the patch, because with the new __niter_base
overload the debug layer gets removed. Your patch kept the checking by
not removing the debug layer, but loses the memcmp optimization.
My suggestion above removes the debug layer so uses the optimization,
but adds a separate range check, so we still get checking as well.
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());
We should assert for that test, it's undefined behaviour.
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.
OK great.
