RFR: 8196106: Support nested infinite or recursive flat mapped streams [v7]

[Viktor Klang]

On Sat, 13 Apr 2024 09:04:36 GMT, Rémi Forax <forax at openjdk.org> wrote:

>> Viktor Klang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Adding additional, short-circuit-specific, cases to the FlatMap benchmark
>
> src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 442:
> 
>> 440:      *         {@code false} if not.
>> 441:      */
>> 442:     protected final boolean isShortCircuitingPipeline() {
> 
> protected can be replaced by package-private

Since `hasAnyStateful()` is protected, I think this too should be, for consistency.

> src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 448:
> 
>> 446:              u = u.nextStage) {
>> 447:         }
>> 448:         return result;
> 
> can be written in a simpler way
> 
> for(var stage = sourceStage.nextStage; stage != null; stage = stage.nextStage) {
>   if (StreamOpFlag.SHORT_CIRCUIT.isKnown(u.combinedFlags)) {
>     return true;
>   }
>   return false;
> }
> 
> no local variable and no SSA phi

In general, I don't like multiple exit-paths from methods, but I agree that in this case it increases readability.

> src/java.base/share/classes/java/util/stream/DoublePipeline.java line 267:
> 
>> 265:             @Override
>> 266:             Sink<Double> opWrapSink(int flags, Sink<Double> sink) {
>> 267:                 final DoubleConsumer fastPath =
> 
> this `final` is unecessary, inconsistent with the style of all other fiels of this package.

I think it is good practice to `final` anything which will be closed over. (Effectively Final is not as clear as Explicitly Final)

> src/java.base/share/classes/java/util/stream/DoublePipeline.java line 281:
> 
>> 279:                     @Override
>> 280:                     public void accept(double e) {
>> 281:                         try (final var result = mapper.apply(e)) {
> 
> `final` is unecessary and i will keep the type instead of `var` because this code quite complex and eliding the type does not help (and also mixing var and not var in the same method is not recommanded cf Stuart guide on where using 'var')

There's only a single var in this method, and no other variables. But I agree on keeping the type explicit, since the definition of the `mapper` is far away from the application thereof.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1564050082
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1564051078
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1564051678
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1564052465