On Sat, 13 Apr 2024 09:04:36 GMT, Rémi Forax <fo...@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
