On Thu, 9 Nov 2023 08:15:02 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> Viktor Klang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Addressing review feedback
>> - Make Gatherer.andThen take a wildcard for the rhs Gatherer state type
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 261:
>
>> 259: /**
>> 260: * Returns an initializer which is the default initializer of a
>> Gatherer.
>> 261: * The returned initializer identifies that the owner Gatherer is
>> stateless.
>
> I know the text is logically correct but is there a way to be more
> intuitively correct? I mean, there are other initializers that could also
> mean the Gather is stateless.
@minborg Not really, from an evaluation-perspective only this specific
initializer indicates true statelessness (other instances will signal that it
is stateful even if it happens to use `null` as its state). True statelessness
means that the initializer doesn't need to be invoked, and that any state does
not need to be tracked.
> src/java.base/share/classes/java/util/stream/Gatherer.java line 267:
>
>> 265: * @param <A> the type of the state of the returned initializer
>> 266: */
>> 267: static <A> Supplier<A> defaultInitializer() {
>
> Would code sharing imply performance implications due to profile pollution?
I guess that depends how deep from the callsite the profiler keeps information
and how much of it gets inlined.
> src/java.base/share/classes/java/util/stream/Gatherer.java line 419:
>
>> 417: * @return the new {@code Gatherer}
>> 418: */
>> 419: static <T, R> Gatherer<T, Void, R> of(
>
> Nit: In `Collector` the parameters are formatted in a different way. Should
> we be consistent with that existing formatting?
Which parameters? The type parameters or the method parameters?
> src/java.base/share/classes/java/util/stream/Gatherer.java line 535:
>
>> 533: */
>> 534: @ForceInline
>> 535: static <A, T, R> Integrator<A, T, R> of(Integrator<A, T, R>
>> integrator) {
>
> While this idiom is very convenient, there are some trap doors in
> combinations with method references and compatibility which is why we backed
> out of https://github.com/openjdk/jdk/pull/16213
>
> It is good that the text mentions "lambda" but maybe we should explicitly
> mention that method references might introduce said problems?
This idiom is safer than casts since the casts are blind and need
unchecked-annotations. I'm willing to take this approach for an evaluation
during the Preview phase.
> src/java.base/share/classes/java/util/stream/Gatherers.java line 68:
>
>> 66: */
>> 67: @SuppressWarnings("rawtypes")
>> 68: enum Value implements Supplier, BinaryOperator, BiConsumer {
>
> Can we move down this class at the end of Gatherers as it is an
> implementation concern? Or even better. move it to jdk.internal
What specifically are you looking to achieve?
> src/java.base/share/classes/java/util/stream/Gatherers.java line 326:
>
>> 324:
>> 325: /**
>> 326: * Gathers elements into fixed-size windows. The last window may
>> contain
>
> Returns a ...
You mean "Returns a Gatherer which gathers elements into ..." ?
> src/java.base/share/classes/java/util/stream/Gatherers.java line 347:
>
>> 345: * @throws IllegalArgumentException when windowSize is less than 1
>> 346: */
>> 347: public static <TR> Gatherer<TR, ?, List<TR>> windowFixed(int
>> windowSize) {
>
> In my opinion, it would be nicer to let `Gatherers` be a shopping window for
> cool gatherers. Ideally, I think only the docs, methods, parameters, and
> invariant assertions should be visible here. The rest could be tucked away
> under the covers.
@minborg While I think that would be fantastic from a theoretical standpoint --
however, as we've already seen with windowSliding and windowFixed, the need for
performance and correctness will mean that the Gatherer implementations herein
will likely not be "short and nice" which would be ideal from a "shopping
window" perspective. And remember, it is not always the case that the cheese in
the window is real. :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387726780
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387728774
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387730338
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387733407
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387734060
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387737838
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387736978
