On Fri, 10 Nov 2023 09:32:17 GMT, Viktor Klang <vkl...@openjdk.org> wrote:
>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461) > > Viktor Klang has updated the pull request incrementally with two additional > commits since the last revision: > > - Multiple improvements to Gatherer and Gatherers javadoc and restructuring > of Gatherers.java to put public at the top of the file. > - Augmenting Gatherer tests to include default implementation in Stream Very nice API. Some minor suggestions and comments (and one fix for one of the examples). I only looked at the API. src/java.base/share/classes/java/util/stream/Gatherer.java line 45: > 43: * or be parallelized -- if a combiner function is supplied. > 44: * > 45: * <p>Examples of gathering operations include, but is not limited to: > Examples of ... but _are_ not limited to src/java.base/share/classes/java/util/stream/Gatherer.java line 115: > 113: * Gatherer<Object, ?, String> toString = map(i -> i.toString()); > 114: * > 115: * Gatherer<Integer, ?, String> incrementThenToString = > plusOne.andThen(intToString); Suggestion: * Gatherer<Integer, ?, String> incrementThenToString = increment.andThen(intToString); src/java.base/share/classes/java/util/stream/Gatherer.java line 138: > 136: * ); > 137: * } > 138: * } I would have appreciated a usage example here - with a stream of input elements and what such a gatherer would produce as output. src/java.base/share/classes/java/util/stream/Gatherer.java line 250: > 248: * @param that the other gatherer > 249: * @param <RR> The type of output of that Gatherer > 250: * @throws NullPointerException if the argument is null Suggestion: * @throws NullPointerException if the argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 312: > 310: * @param <T> the type of input elements for the new gatherer > 311: * @param <R> the type of results for the new gatherer > 312: * @throws NullPointerException if the argument is null Suggestion: * @throws NullPointerException if the argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 333: > 331: * @param <T> the type of input elements for the new gatherer > 332: * @param <R> the type of results for the new gatherer > 333: * @throws NullPointerException if any argument is null Suggestion: * @throws NullPointerException if any argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 356: > 354: * @param <A> the type of initializer for the new gatherer > 355: * @param <R> the type of results for the new gatherer > 356: * @throws NullPointerException if any argument is null Suggestion: * @throws NullPointerException if any argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 402: > 400: * @param <T> the type of input elements for the new gatherer > 401: * @param <R> the type of results for the new gatherer > 402: * @throws NullPointerException if any argument is null Suggestion: * @throws NullPointerException if any argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 422: > 420: * @param <T> the type of input elements for the new gatherer > 421: * @param <R> the type of results for the new gatherer > 422: * @throws NullPointerException if any argument is null Suggestion: * @throws NullPointerException if any argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 448: > 446: * @param <A> the type of initializer for the new gatherer > 447: * @param <R> the type of results for the new gatherer > 448: * @throws NullPointerException if any argument is null Suggestion: * @throws NullPointerException if any argument is {@code null} src/java.base/share/classes/java/util/stream/Gatherer.java line 492: > 490: * > 491: * @apiNote This is best-effort only, once this returns true it > should > 492: * never return false again for the same instance. Suggestion: * @apiNote This is best-effort only, once this returns {@code true} it should * never return {@code false} again for the same instance. src/java.base/share/classes/java/util/stream/Gatherer.java line 577: > 575: interface Greedy<A, T, R> extends Integrator<A, T, R> { } > 576: } > 577: } Looks like there is no newline at the end of this file. Though newline at end of file is not mandatory it avoids getting warning in some tools (such as IDE etc...) Suggestion: } src/java.base/share/classes/java/util/stream/Gatherers.java line 80: > 78: * and the contents of the windows it produces > 79: * @return a new gatherer which groups elements into fixed-size > windows > 80: * @throws IllegalArgumentException when windowSize is less than 1 Suggestion: * @throws IllegalArgumentException when {@code windowSize} is less than 1 src/java.base/share/classes/java/util/stream/Gatherers.java line 164: > 162: * and the contents of the windows it produces > 163: * @return a new gatherer which groups elements into sliding windows > 164: * @throws IllegalArgumentException when windowSize is less than 1 Suggestion: * @throws IllegalArgumentException when {@code windowSize} is less than 1 src/java.base/share/classes/java/util/stream/Gatherers.java line 235: > 233: * <p>Example: > 234: * {@snippet lang = java: > 235: * // will contain: Optional[123456789] Suggestion: * // will contain: Optional["123456789"] src/java.base/share/classes/java/util/stream/Gatherers.java line 251: > 249: * @param <R> the type of elements the returned gatherer produces > 250: * @return a new Gatherer > 251: * @throws NullPointerException if any of the parameters are null Suggestion: * @throws NullPointerException if any of the parameters are {@code null} src/java.base/share/classes/java/util/stream/Gatherers.java line 284: > 282: * <p>Example: > 283: * {@snippet lang = java: > 284: * // will contain: [1, 12, 123, 1234, 12345, 123456, 1234567, > 12345678, 123456789] Suggestion: * // will contain: ["1", "12", "123", "1234", "12345", "123456", "1234567", "12345678", "123456789"] src/java.base/share/classes/java/util/stream/Gatherers.java line 298: > 296: * @param <R> the type of element which this gatherer produces > 297: * @return a new Gatherer which performs a prefix scan > 298: * @throws NullPointerException if any of the parameters are null Suggestion: * @throws NullPointerException if any of the parameters are {@code null} src/java.base/share/classes/java/util/stream/Gatherers.java line 320: > 318: * An operation which executes operations concurrently > 319: * with a fixed window of max concurrency, using > 320: * <a > href="{@docRoot}/java.base/java/lang/Thread.html#virtual-threads">virtual > threads</a>. Suggestion: * {@linkplain Thread##virtual-threads virtual threads}. src/java.base/share/classes/java/util/stream/Gatherers.java line 337: > 335: * @return a new Gatherer > 336: * @throws IllegalArgumentException if maxConcurrency is less than 1 > 337: * @throws NullPointerException if mapper is null Suggestion: * @throws IllegalArgumentException if {@code maxConcurrency} is less than 1 * @throws NullPointerException if mapper is {@code null} ------------- PR Review: https://git.openjdk.org/jdk/pull/16420#pullrequestreview-1725219596 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389667892 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389673357 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389677998 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389682609 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389685672 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389687482 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389687891 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389688993 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389689868 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389690634 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389691810 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389693254 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389696394 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389698730 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389700158 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389700567 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389701449 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389701795 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389702802 PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389703538
