On Mon, 24 May 2021 07:15:34 GMT, Tagir F. Valeev <tval...@openjdk.org> wrote:
>> Sometimes, `Spliterator::forEachRemaining` has much more optimized >> implementation, compared to `Spliterator::tryAdvance`. For example, if a >> `Stream::spliterator` called for a stream that has intermediate operations, >> `tryAdvance()` may buffer intermediate elements while `forEachRemaining()` >> just delegates to the `wrapAndCopyInto` (see >> `StreamSpliterators.AbstractWrappingSpliterator` and its inheritors). >> >> `Spliterators::iterator` methods (used in particular by `Stream::iterator`) >> provide adapter iterators that delegate to the supplied spliterator. >> Unfortunately, they don't have a specialized implementation for >> `Iterator::forEachRemaining`. As a result, a default implementation is used >> that delegates to `hasNext`/`next`, which in turn causes the delegation to >> `tryAdvance`. It's quite simple to implement `Iterator::forEachRemaining` >> here, taking advantage of possible spliterator optimizations if the iterator >> client decides to use `forEachRemaining`. >> >> This patch implements Iterator::forEachRemaining in Spliterators::iterator >> methods. Also, I nullize the `nextElement` in `Iterator::next` to avoid >> accidental prolongation of the user's object lifetime when iteration is >> finished. Finally, a small cleanup: I added the `final` modifier where >> possible to private fields in `Spliterators`. >> >> Test-wise, some scenarios are already covered by >> SpliteratorTraversingAndSplittingTest. However, the resulting iterator is >> always wrapped into `Spliterators::spliterator`, so usage scenarios are >> somewhat limited. In particular, calling `hasNext` (without `next`) before >> `forEachRemaining` was not covered there. I added more tests in >> `IteratorFromSpliteratorTest` to cover these scenarios. I checked that >> removing `valueReady = false;` or `action.accept(t);` lines from newly >> implemented `forEachRemaining` method causes new tests to fail (but old >> tests don't fail due to this). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Test: formatting; tests for empty spliterator Oops i missed the bit about testing in the description. That's subtle, the `hasNext` occurs in the assert before the `forEachRemaining`. Could you add a comment that this is the primary purpose of the test since `SpliteratorTraversingAndSplittingTest` cannot do that for a spliterator wrapping an iterator? ------------- Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4124
