[10] RFR 8075939: Stream.flatMap() causes breaking of short-circuiting of terminal operations

[Paul Sandoz]

Hi,

Please review the following webrev that makes flatMap non-aggressive when pushing elements downstream if any downstream operation is short-circuiting.

  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8075939-flatMap-aggressive-push/webrev/index.html <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8075939-flatMap-aggressive-push/webrev/index.html>

This enables support for flat mapping to an infinite stream (assuming there is a downstream short-circuiting operation to terminate the stream computation).

Thanks,
Paul.

[Remi Forax]

Hi Paul,
three things:
- I think you should add a comment to explain why you have chosen to create a the field downstream* in the primitive implementations,
  I suppose it's to avoid to allocate a lambda proxy at each call.
- the fields in the inner classes cancellationRequested and downstream* should be private.
- if you use var, you should use a meaningful name, here, 's' can be replaced by 'spliterator', making the code more readable.

cheers,
Rémi  

----- Mail original -----
> De: "Paul Sandoz" <[hidden email]>
> À: "core-libs-dev" <[hidden email]>
> Envoyé: Jeudi 21 Décembre 2017 23:40:18
> Objet: [10] RFR 8075939: Stream.flatMap() causes breaking of short-circuiting of terminal operations

> Hi,
>
> Please review the following webrev that makes flatMap non-aggressive when
> pushing elements downstream if any downstream operation is short-circuiting.
>
>  http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8075939-flatMap-aggressive-push/webrev/index.html
>  <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8075939-flatMap-aggressive-push/webrev/index.html>
>
> This enables support for flat mapping to an infinite stream (assuming there is a
> downstream short-circuiting operation to terminate the stream computation).
>
> Thanks,
> Paul.

[Paul Sandoz]

> On 21 Dec 2017, at 15:46, Remi Forax <[hidden email]> wrote:
>
> Hi Paul,
> three things:
> - I think you should add a comment to explain why you have chosen to create a the field downstream* in the primitive implementations,
>  I suppose it's to avoid to allocate a lambda proxy at each call.

Yes, i included this comment:
// cache the consumer to avoid creation on every accepted element

> - the fields in the inner classes cancellationRequested and downstream* should be private.

Does it make any difference for such inner classes? (I lean towards package private as the default.)


> - if you use var, you should use a meaningful name, here, 's' can be replaced by 'spliterator', making the code more readable.
>

Don’t agree in this case, given the locality of use and given the method name on the RHS.

Thanks,
Paul.

[Remi Forax]

> De: "Paul Sandoz" <[hidden email]>
> À: "Remi Forax" <[hidden email]>
> Cc: "core-libs-dev" <[hidden email]>
> Envoyé: Vendredi 22 Décembre 2017 01:38:37
> Objet: Re: [10] RFR 8075939: Stream.flatMap() causes breaking of
> short-circuiting of terminal operations

>> On 21 Dec 2017, at 15:46, Remi Forax < [ mailto:[hidden email] |
>> [hidden email] ] > wrote:

>> Hi Paul,
>> three things:
>> - I think you should add a comment to explain why you have chosen to create a
>> the field downstream* in the primitive implementations,
>> I suppose it's to avoid to allocate a lambda proxy at each call.

> Yes, i included this comment:
> // cache the consumer to avoid creation on every accepted element

>> - the fields in the inner classes cancellationRequested and downstream* should
>> be private.

> Does it make any difference for such inner classes?

Usually, a stronger encapsulation is better than a weaker one. Being an anonymous class only means that you can not access the type, not that the fields are not accessible.

> (I lean towards package private as the default.)

I tend to think that before, but with nestmate around the corner, private now really means accessible in the same java file, so i do think that private should be the new default in nested classes.

>> - if you use var, you should use a meaningful name, here, 's' can be replaced by
>> 'spliterator', making the code more readable.

> Don’t agree in this case, given the locality of use and given the method name on
> the RHS.

It means that when you want to know what a variable is you have to read its initialization part, it's a bit of a stretch compare to what Brian said about var, we can use var because the variable name is meaningful enough, so the type is redundant. I suppose it's not a big deal if you read the code sequentially but i tend to read the meat part of the code and extend above and below, so i was focus on the code that uses downstreamAsInt when i ask myself what 's' was.

> Thanks,
> Paul.

cheers,
Rémi

[Paul Sandoz]

Hi Remi,

Catching up after the holidays. I would prefer to leave both things as they are.

Thanks,
Paul.

> On 22 Dec 2017, at 03:15, [hidden email] wrote:
>
>
>
> De: "Paul Sandoz" <[hidden email]>
> À: "Remi Forax" <[hidden email]>
> Cc: "core-libs-dev" <[hidden email]>
> Envoyé: Vendredi 22 Décembre 2017 01:38:37
> Objet: Re: [10] RFR 8075939: Stream.flatMap() causes breaking of short-circuiting of terminal operations
>
>
> On 21 Dec 2017, at 15:46, Remi Forax <[hidden email]> wrote:
>
> Hi Paul,
> three things:
> - I think you should add a comment to explain why you have chosen to create a the field downstream* in the primitive implementations,
>  I suppose it's to avoid to allocate a lambda proxy at each call.
>
> Yes, i included this comment:
> // cache the consumer to avoid creation on every accepted element
>
> - the fields in the inner classes cancellationRequested and downstream* should be private.
>
> Does it make any difference for such inner classes?
>
> Usually, a stronger encapsulation is better than a weaker one.  Being an anonymous class only means that you can not access the type, not that the fields are not accessible.
>
> (I lean towards package private as the default.)
>
> I tend to think that before, but with nestmate around the corner, private now really means accessible in the same java file, so i do think that private should be the new default in nested classes.
>
>
>
> - if you use var, you should use a meaningful name, here, 's' can be replaced by 'spliterator', making the code more readable.
>
>
> Don’t agree in this case, given the locality of use and given the method name on the RHS.
>
> It means that when you want to know what a variable is you have to read its initialization part, it's a bit of a stretch compare to what Brian said about var, we can use var because the variable name is meaningful enough, so the type is redundant. I suppose it's not a big deal if you read the code sequentially but i tend to read the meat part of the code and extend above and below, so i was focus on the code that uses downstreamAsInt when i ask myself what 's' was.
>
>
> Thanks,
> Paul.
>
> cheers,
> Rémi
>

[Remi Forax]

sure, if you prefer, it's fine for me.

Remi


On January 12, 2018 6:09:27 PM UTC, Paul Sandoz <[hidden email]> wrote:

>Hi Remi,
>
>Catching up after the holidays. I would prefer to leave both things as
>they are.
>
>Thanks,
>Paul.
>
>> On 22 Dec 2017, at 03:15, [hidden email] wrote:
>>
>>
>>
>> De: "Paul Sandoz" <[hidden email]>
>> À: "Remi Forax" <[hidden email]>
>> Cc: "core-libs-dev" <[hidden email]>
>> Envoyé: Vendredi 22 Décembre 2017 01:38:37
>> Objet: Re: [10] RFR 8075939: Stream.flatMap() causes breaking of
>short-circuiting of terminal operations
>>
>>
>> On 21 Dec 2017, at 15:46, Remi Forax <[hidden email]> wrote:
>>
>> Hi Paul,
>> three things:
>> - I think you should add a comment to explain why you have chosen to
>create a the field downstream* in the primitive implementations,
>>  I suppose it's to avoid to allocate a lambda proxy at each call.
>>
>> Yes, i included this comment:
>> // cache the consumer to avoid creation on every accepted element
>>
>> - the fields in the inner classes cancellationRequested and
>downstream* should be private.
>>
>> Does it make any difference for such inner classes?
>>
>> Usually, a stronger encapsulation is better than a weaker one.  Being
>an anonymous class only means that you can not access the type, not
>that the fields are not accessible.
>>
>> (I lean towards package private as the default.)
>>
>> I tend to think that before, but with nestmate around the corner,
>private now really means accessible in the same java file, so i do
>think that private should be the new default in nested classes.
>>
>>
>>
>> - if you use var, you should use a meaningful name, here, 's' can be
>replaced by 'spliterator', making the code more readable.
>>
>>
>> Don’t agree in this case, given the locality of use and given the
>method name on the RHS.
>>
>> It means that when you want to know what a variable is you have to
>read its initialization part, it's a bit of a stretch compare to what
>Brian said about var, we can use var because the variable name is
>meaningful enough, so the type is redundant. I suppose it's not a big
>deal if you read the code sequentially but i tend to read the meat part
>of the code and extend above and below, so i was focus on the code that
>uses downstreamAsInt when i ask myself what 's' was.
>>
>>
>> Thanks,
>> Paul.
>>
>> cheers,
>> Rémi
>>

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.