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. |
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. |
> 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. |
> 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 |
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 > |
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. |
Free forum by Nabble | Edit this page |