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

[Remi Forax]

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

Remi


On January 12, 2018 6:09:27 PM UTC, Paul Sandoz <paul.san...@oracle.com> 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, fo...@univ-mlv.fr wrote:
>> 
>> 
>> 
>> De: "Paul Sandoz" <paul.san...@oracle.com>
>> À: "Remi Forax" <fo...@univ-mlv.fr>
>> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
>> 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 <fo...@univ-mlv.fr> 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.

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

[forax]

> De: "Paul Sandoz" <paul.san...@oracle.com>
> À: "Remi Forax" <fo...@univ-mlv.fr>
> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
> 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:fo...@univ-mlv.fr |
>> fo...@univ-mlv.fr ] > 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 

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

[Paul Sandoz]

> On 21 Dec 2017, at 15:46, Remi Forax  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.

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

[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" <paul.san...@oracle.com>
> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>
> 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.

[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
 


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.