Hello! Here's updated webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r3/ All mentioned issues were fixed. I agree that using new features like List.of is better, because this way it's easier to find problems with them prior to release.
With best regards, Tagir Valeev. On Mon, Feb 22, 2016 at 2:50 PM, Paul Sandoz <paul.san...@oracle.com> wrote: > Hi Tagir, > > > On 21 Feb 2016, at 13:15, Tagir Valeev <amae...@gmail.com> wrote: > > > > 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. > > > > Looking good. Just minor comments. > > For the primitive streams > — > > + Spliterator.OfDouble spliterator = new > Spliterators.AbstractDoubleSpliterator(Long.MAX_VALUE, > + Spliterator.ORDERED | Spliterator.IMMUTABLE) { > + double prev; > ... > - return > StreamSupport.doubleStream(Spliterators.spliteratorUnknownSize( > - iterator, > - Spliterator.ORDERED | Spliterator.IMMUTABLE | > Spliterator.NONNULL), false); > > We should retain the NONNULL characteristic. > > Thinking a little more that is something that should perhaps be added by > default by the primitive abstract spliterators. Something to consider later > maybe. > > > Stream > — > > public static<T> Stream<T> iterate(final T seed, final > UnaryOperator<T> f) { > Objects.requireNonNull(f); > > - final Iterator<T> iterator = new Iterator<T>() { > - @SuppressWarnings("unchecked") > - T t = (T) Streams.NONE; > > I believe you can now remove the field Streams.NONE? > > > 1186 public static<T> Stream<T> iterate(final T seed, final > UnaryOperator<T> f) { > 1187 Objects.requireNonNull(f); > 1188 Spliterator<T> spliterator = new > Spliterators.AbstractSpliterator<T>(Long.MAX_VALUE, > 1189 Spliterator.ORDERED | Spliterator.IMMUTABLE) { > > I forgot to mention before you can now use the “<>" on the RHS, although > IDEs might not like it javac will :-) > > > 1270 @Override > 1271 public void forEachRemaining(Consumer<? super T> action) { > 1272 Objects.requireNonNull(action); > 1273 if (finished) > 1274 return; > 1275 T t = started ? f.apply(prev) : seed; > 1276 finished = true; > > A really minor thing but if you move “finished = true” above one line, it > means there is one less place where unintended recursive traversal can > occur. > > > IterateTest > — > > 57 {Arrays.asList(1, 2, 4, 8, 16, 32, 64, 128, 256, 512), > 58 Factory.ofSupplier("ref.ten", () -> Stream.iterate(1, > x -> x < 1000, x -> x * 2))}, > > We now have the method “List.of", up to you if you wanna update to use > that or not. > > > Thanks, > Paul. > > >
