Thank you for valueable coments, here's updated webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r2/
Changes: - Spec updated according to Brian Goetz suggestion - Predicate<? super T> wildcard - two-arg versions of iterate() are rewritten with Spliterator and started flag - IterateTest rewritten: now withData().stream().expectedResult().exercize() used; expected results added. With best regards, Tagir Valeev On Tue, Feb 16, 2016 at 7:32 PM, Paul Sandoz <paul.san...@oracle.com> wrote: > Hi Tagir, > > The implementation approach looks good. I agree with Brian on the > documentation, esp. about the connection to a for loop. > > Testing-wise adding to the spliterator data sources is ok. Because you > have done that there is no need to explicitly use SpliteratorTestHelper in > IterateTest, as that is reasonably covered by SpliteratorTest. Since you > have the data providers for streams just run it through exerciseStream > instead. > > Regarding wild cards we could clean this up later and include other > methods [1]. It’s easy to go overboard on this, recommend sticking to ? > extends/? super where appropriate. The existing iterate methods use > UnaryOperator, thus there is no wiggle room there, am i ok with reusing > that for the 3-arg method. > > Regarding the existing two-args method, my inclination is to keep the > implementations separate from the 3-args, but feel free to change to using > abstract spliterator (reduce layering) using a similar code pattern (no > need for "finished" or overriding forEachRemaining). Then you can remove > Streams.NONE and it’s sneaky erased usage :-) > > Paul. > > [1] https://bugs.openjdk.java.net/browse/JDK-8132097 > > > On 14 Feb 2016, at 15:53, Tagir F. Valeev <amae...@gmail.com> wrote: > > > > Hello! > > > > I wanted to work on foldLeft, but Brian asked me to take this issue > > instead. So here's webrev: > > http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r1/ > > > > I don't like iterator-based Stream source implementations, so I made > > them AbstractSpliterator-based. I also implemented manually > > forEachRemaining as, I believe, this improves the performance in > > non-short-circuiting cases. > > > > I also decided to keep two flags (started and finished) to track the > > state. Currently existing implementation of infinite iterate() does > > not use started flag, but instead reads one element ahead for > > primitive streams. This seems wrong to me and may even lead to > > unexpected exceptions (*). I could get rid of "started" flag for > > Stream.iterate() using Streams.NONE, but this would make object > > implementation different from primitive implementations. It would also > > be possible to keep single three-state variable (byte or int, > > NOT_STARTED, STARTED, FINISHED), but I doubt that this would improve > > the performance or footprint. Having two flags looks more readable to > > me. > > > > Currently existing two-arg iterate methods can now be expressed as a > > partial case of the new method: > > > > public static<T> Stream<T> iterate(final T seed, final UnaryOperator<T> > f) { > > return iterate(seed, x -> true, f); > > } > > (same for primitive streams). I may do this if you think it's > > reasonable. > > > > I created new test class and added new iterate sources to existing > > data providers. > > > > Please review and sponsor! > > > > With best regards, > > Tagir Valeev. > > > > (*) Consider the following code: > > > > int[] data = {1,2,3,4,-1}; > > IntStream.iterate(0, x -> data[x]) > > .takeWhile(x -> x >= 0) > > .forEach(System.out::println); > > > > Currently this unexpectedly throws an AIOOBE, because > > IntStream.iterate unnecessarily tries to read one element ahead. > > > >
