On 01/02/21 8:09 pm, Jonathan Wakely wrote:
On 01/02/21 19:30 +0100, François Dumont via Libstdc++ wrote:
On 01/02/21 6:43 pm, Jonathan Wakely wrote:
On 31/01/21 16:59 +0100, François Dumont via Libstdc++ wrote:
After the debug issue has been fixed in PR 98466 the problem was
not in the debug iterator implementation itself but in the deque
iterator operator- implementation.
libstdc++: Make deque iterator operator- usable with value-init
iterators
N3644 implies that operator- can be used on value-init
iterators. We now return
0 if both iterators are value initialized. If only one is value
initialized we
keep the UB by returning the result of a normal computation
which is an unexpected
value.
libstdc++/ChangeLog:
PR libstdc++/70303
* include/bits/stl_deque.h
(std::deque<>::operator-(iterator, iterator)):
Return 0 if both iterators are value-initialized.
* testsuite/23_containers/deque/70303.cc: New test.
* testsuite/23_containers/vector/70303.cc: New test.
Tested under Linux x86_64, ok to commit ?
OK.
I don't like adding the branch there though. Even with the
__builtin_expect it causes worse code to be generated than the
original.
This would be branchless, but a bit harder to understand:
return difference_type(__x._S_buffer_size())
* (__x._M_node - __y._M_node - int(__x._M_node == __y._M_node))
+ (__x._M_cur - __x._M_first) + (__y._M_last - __y._M_cur);
Please commit the fix and we can think about it later.
I do not think this work because for value-init iterators we have
__x._M_node == __y._M_node == nullptr so the diff would give
-_S_buffer_size().
But I got the idear, I'll prepare the patch.
Yeah, I just came back to the computer to say that it's wrong. But
maybe this:
return difference_type(_S_buffer_size())
* (__x._M_node - __y._M_node - int(__x._M_node && __y._M_node))
+ (__x._M_cur - __x._M_first) + (__y._M_last - __y._M_cur);
For value-init'd iterators we'd get _S_buffer_size() * 0 + 0 - 0
We could even just use int(__x._M_node != 0) because if one is null
and the other isn't, it's UB anyway.
This is what I've tested with success. As it is a recent kind of
regression you might want to consider it now.
libstdc++: Remove execution branch in deque iterator operator-
libstdc++-v3/ChangeLog:
* include/bits/stl_deque.h
(std::operator-(deque::iterator, deque::iterator)): Replace
if/then with
a null pointer test.
Ok to commit ?
François
diff --git a/libstdc++-v3/include/bits/stl_deque.h b/libstdc++-v3/include/bits/stl_deque.h
index 04b70b77621..8bba7a3740f 100644
--- a/libstdc++-v3/include/bits/stl_deque.h
+++ b/libstdc++-v3/include/bits/stl_deque.h
@@ -352,12 +352,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
friend difference_type
operator-(const _Self& __x, const _Self& __y) _GLIBCXX_NOEXCEPT
{
- if (__builtin_expect(__x._M_node || __y._M_node, true))
- return difference_type(_S_buffer_size())
- * (__x._M_node - __y._M_node - 1) + (__x._M_cur - __x._M_first)
- + (__y._M_last - __y._M_cur);
-
- return 0;
+ return difference_type(_S_buffer_size())
+ * (__x._M_node - __y._M_node - int(__x._M_node != 0))
+ + (__x._M_cur - __x._M_first)
+ + (__y._M_last - __y._M_cur);
}
// _GLIBCXX_RESOLVE_LIB_DEFECTS
@@ -369,12 +367,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
operator-(const _Self& __x,
const _Deque_iterator<_Tp, _RefR, _PtrR>& __y) _GLIBCXX_NOEXCEPT
{
- if (__builtin_expect(__x._M_node || __y._M_node, true))
- return difference_type(_S_buffer_size())
- * (__x._M_node - __y._M_node - 1) + (__x._M_cur - __x._M_first)
- + (__y._M_last - __y._M_cur);
-
- return 0;
+ return difference_type(_S_buffer_size())
+ * (__x._M_node - __y._M_node - int(__x._M_node != 0))
+ + (__x._M_cur - __x._M_first)
+ + (__y._M_last - __y._M_cur);
}
friend _Self
