The standard specifies some of the effects of ranges::advance in terms
of "Equivalent to:" and it's observable that our current implementation
deviates from the precise specification in the standard. This was
causing some failures in the libc++ testsuite.
For the sized_sentinel_for<I, S> case I optimized our implementation to
avoid redundant calls when we have already checked that there's nothing
to do. We were eliding `advance(i, bound)` when the iterator already
equals the sentinel, and eliding `advance(i, n)` when `n` is zero. In
both cases, removing the seemingly redundant calls is not equivalent to
the spec because `i = std::move(bound)` or `i += 0` operations can be
observed by program-defined iterators. This patch inlines the observable
side effects of advance(i, bound) or advance(i, 0) without actually
calling those functions.
For the non-sized sentinel case, `if (i == bound || n == 0)` is
different from `if (n == 0 || i == bound)` for the case where n is zero
and a program-defined iterator observes the number of comparisons.
This patch changes it to do `n == 0` first. I don't think this is
required by the standard, as this condition is not "Equivalent to:" any
observable sequence of operations, but testing `n == 0` first is
probably cheaper anyway.
libstdc++-v3/ChangeLog:
* include/bits/ranges_base.h (ranges::advance(i, n, bound)):
Ensure that observable side effects on iterators match what is
specified in the standard.
---
For most iterator types, I assume the compiler will inline these extra
redundant operations, so they'll only make a difference for iterators
that do actually observe the number of operations.
Tested powerpc64le-linux.
libstdc++-v3/include/bits/ranges_base.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/libstdc++-v3/include/bits/ranges_base.h
b/libstdc++-v3/include/bits/ranges_base.h
index 0251e5d0928a..4a0b9e2f5ec3 100644
--- a/libstdc++-v3/include/bits/ranges_base.h
+++ b/libstdc++-v3/include/bits/ranges_base.h
@@ -882,7 +882,14 @@ namespace ranges
const auto __diff = __bound - __it;
if (__diff == 0)
- return __n;
+ {
+ // inline any possible side effects of advance(it, bound)
+ if constexpr (assignable_from<_It&, _Sent>)
+ __it = std::move(__bound);
+ else if constexpr (random_access_iterator<_It>)
+ __it += 0;
+ return __n;
+ }
else if (__diff > 0 ? __n >= __diff : __n <= __diff)
{
(*this)(__it, __bound);
@@ -897,9 +904,14 @@ namespace ranges
return 0;
}
else
- return 0;
+ {
+ // inline any possible side effects of advance(it, n)
+ if constexpr (random_access_iterator<_It>)
+ __it += 0;
+ return 0;
+ }
}
- else if (__it == __bound || __n == 0)
+ else if (__n == 0 || __it == __bound)
return __n;
else if (__n > 0)
{
--
2.50.1
