On Fri, 10 Nov 2023 16:48:34 GMT, Alan Bateman <[email protected]> wrote:
>> 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
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 91:
>
>> 89: *
>> 90: * <p>In addition to the predefined implementations in {@link
>> Gatherers}, the
>> 91: * static factory methods {@code of(...)} and {@code ofSequential(...)}
>
> I assume both of these could be links.
Unfortunately they can't, since they are both overloaded and here we're talking
about them "generically".
> src/java.base/share/classes/java/util/stream/Gatherer.java line 494:
>
>> 492: * never return false again for the same instance.
>> 493: *
>> 494: * <p>By default this method returns {@code false}.
>
> The text that specifies the default implementation can be in implSpec rather
> than the apiNote. I agree that false is the only sane default here.
Yeah, should likely be implSpec. Will change.
> src/java.base/share/classes/java/util/stream/Gatherer.java line 528:
>
>> 526: * {@code false} if not
>> 527: */
>> 528: boolean integrate(A state, T element, Downstream<? super R>
>> downstream);
>
> Integrator is silent on nulls and whether NPE is thrown.
No sure what it should say regarding nulls. The `initializer` might return
`null` in which case `state` would be `null`. `element` can be `null` if the
stream contains `null`s. `Downstream` should never be null.
Should the Javadoc say something about how RuntimeExceptions thrown must be
interpreted by the caller? 🤔
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389957565
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389956115
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1389957272
