On Wed, 1 Nov 2023 16:16:38 GMT, Brian Goetz <briango...@openjdk.org> wrote:
>> This is a Draft PR for [JEP-461](https://openjdk.org/jeps/461) > > src/java.base/share/classes/java/util/stream/Gatherer.java line 137: > >> 135: * }) >> 136: * ); >> 137: * } > > As a matter of style, I find it weird that `current` is assigned in both the > constructor and in the lambda. I'd be inclined to put all assignments in the > State class (and expose more methods) or put all assignments in the lambdas > (and ditch the ctor). For an example this trivial, it doens't make so much > differnce, but it gives a gentle push towards doing things more consistenly. making `integrate` a method on State in this case is a bit of extra noise, which I wanted to avoid for something embedded in the Javadoc. I'll reduce some of the noise by assigning `current` in the member declaration instead of in an explicit ctor. > src/java.base/share/classes/java/util/stream/Gatherers.java line 516: > >> 514: * evaluation can be out-of-sequence compared to the sequential >> encounter >> 515: * order of the stream. >> 516: * > > How does this differ from Stream::peek? @briangoetz Basically the same as `Stream::parallel().forEach` vs `Stream.parallel().forEachOrdered`. > src/java.base/share/classes/java/util/stream/ReferencePipeline.java line 113: > >> 111: @Override >> 112: final StreamShape getOutputShape() { >> 113: return StreamShape.REFERENCE; > > Not all parameters documented? Good catch -- that was the same for [the other ctor](https://github.com/openjdk/jdk/pull/16420/files/6a7d16a1ffcf26a10ce443cf6bf1f5dd8695b808#diff-761882141e64725ba36ab6cb28dc7ea65deefa32f7b9810355de42126efefc6bR93) so it was carried over, I'll add params for both of the ctors: > src/java.base/share/classes/java/util/stream/Stream.java line 1110: > >> 1108: * }</pre> >> 1109: * >> 1110: * <p>Like {@link #reduce(Object, BinaryOperator)}, {@code >> collect} operations > > A brief description of what a Gatherer is here (around the time you say > "extension point") would be helpful, something general like "Gatherers are > highly flexible, expressing a many-to-many transformation on the elements of > a stream, and support short-circuiting, blah blah blah". ENough to make > people want to read either the linked section or the Gatherer doc. > > The @impleNote is a little unclear; I'd use the format for other ops added > after 8, such as takeWhile or mapMulti. > > I'd add `@see Gatherer`. @briangoetz >A brief description of what a Gatherer is here (around the time you say >"extension point") would be helpful, something general like "Gatherers are >highly flexible, expressing a many-to-many transformation on the elements of a >stream, and support short-circuiting, blah blah blah". ENough to make people >want to read either the linked section or the Gatherer doc. Good point. I went with: `Gatherers are highly flexible and can describe a vast array of possibly stateful operations, with support for short-circuiting, and parallelization.` >The @impleNote is a little unclear; I'd use the format for other ops added >after 8, such as takeWhile or mapMulti. Do you mean the implSpec? Because the implNote just states: "Implementations of this interface should provide their own implementation of this method." >I'd add @see Gatherer. đź‘Ť ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1379142852 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1379143608 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1379102144 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1379120702
