Re: RFR: 8005051: default methods for Iterator

[Zhong Yu]

On Fri, Dec 14, 2012 at 4:54 AM, Remi Forax  wrote:
> Hi Akhil,
>
> On 12/14/2012 02:24 AM, Akhil Arora wrote:
>>
>> As part of the library lambdafication, this patch adds a forEach default
>> method to Iterator, and converts remove() into a default method so that
>> implementations of Iterator no longer have to override remove if they
>> desire the default behavior, which is to throw an
>> UnsupportedOperationException.
>>
>> http://cr.openjdk.java.net/~akhil/8005051.0/webrev/
>>
>> The above patch requires a small patch to an internal class which
>> happens to implement both Iterable and Iterator. Now both Iterable and
>> Iterator supply a default forEach method, so the compiler balks. One
>> minimally intrusive solution is for this class to override both defaults
>> and provide its own version of forEach.
>>
>> http://cr.openjdk.java.net/~akhil/8005053.0/webrev/
>
>
> The issue here is in the code of ASM wich is an external library imported in
> the JDK,
> so not sure it's a good idea to patch it.
>
> Moreover, goggling for "implement Iterable, Iterator" shows that this
> anti-pattern is used too frequently to not step back and see if a better
> solution is not possible.
> https://encrypted.google.com/search?hl=en&q=%22implements+Iterable%3C*%3E%2C+Iterator%3C*%3E%22
>
> We can't remove Collection.forEach without having perf issue because the
> stream pipeline use it.
> Iterator.forEach can be removed but it's a pity because it's really
> convenient,
> a possble solution is to rename Iterator.forEach() to something else.

I agree with renaming the method; it shouldn't be confused with the
typical forEach() on a collection which visits *all* elements; here it
only visits the remaining elements.

>>
>> Please review
>> Thanks
>>
>
> cheers,
> Rémi
>

Re: RFR: 8005051: default methods for Iterator

[Vitaly Davidovich]

One interesting thing for me here is .net's IEnumerable (akin to Iterable)
never received a ForEach extension method - this method only exists on
their List class.  From what I gather this was to avoid passing a block
(Action in .net) that modifies the underlying collection (e.g removes an
item).  I never really thought that paranoia was worth it since most uses
will not want to mutate.  I'm glad Iterable will have this, but curious if
anyone thought about this case explicitly.

Thanks

Sent from my phone
On Dec 14, 2012 8:38 AM, "Peter Levart"  wrote:

> Hi,
>
> Iterator.forEach(Block) does not specify anything about the order of
> internal iteration in correspondence to the order of classical external
> iteration (hasNext()/next()). I think that if the order must be the same
> then Javadoc should mention it. If the orders are allowed be different,
> then Javadoc should mention it too.
>
> As for the clash with Iterable.forEach(Block): The name "forEach" suggests
> just that - each element will be passed to Block. Nothing is said about the
> order of elements or even Threads. I dont't think it should be, since
> Iterables can be various kinds.
>
> But Iterator is a one-shot single-threaded API and it's hard to imagine
> the implementation where internal iteration would want to be different than
> external. So the Iterator method be better called differently. What about
> Iterator.iterate(Block) ?
>
> Regards, Peter
>
> On 12/14/2012 02:24 AM, Akhil Arora wrote:
>
>> As part of the library lambdafication, this patch adds a forEach default
>> method to Iterator, and converts remove() into a default method so that
>> implementations of Iterator no longer have to override remove if they
>> desire the default behavior, which is to throw an
>> UnsupportedOperationException.
>>
>> http://cr.openjdk.java.net/~**akhil/8005051.0/webrev/
>>
>> The above patch requires a small patch to an internal class which happens
>> to implement both Iterable and Iterator. Now both Iterable and Iterator
>> supply a default forEach method, so the compiler balks. One minimally
>> intrusive solution is for this class to override both defaults and provide
>> its own version of forEach.
>>
>> http://cr.openjdk.java.net/~**akhil/8005053.0/webrev/
>>
>> Please review
>> Thanks
>>
>
>

Re: RFR: 8005051: default methods for Iterator

[Peter Levart]

Hi,

Iterator.forEach(Block) does not specify anything about the order of 
internal iteration in correspondence to the order of classical external 
iteration (hasNext()/next()). I think that if the order must be the same 
then Javadoc should mention it. If the orders are allowed be different, 
then Javadoc should mention it too.


As for the clash with Iterable.forEach(Block): The name "forEach" 
suggests just that - each element will be passed to Block. Nothing is 
said about the order of elements or even Threads. I dont't think it 
should be, since Iterables can be various kinds.


But Iterator is a one-shot single-threaded API and it's hard to imagine 
the implementation where internal iteration would want to be different 
than external. So the Iterator method be better called differently. What 
about Iterator.iterate(Block) ?


Regards, Peter

On 12/14/2012 02:24 AM, Akhil Arora wrote:
As part of the library lambdafication, this patch adds a forEach 
default method to Iterator, and converts remove() into a default 
method so that implementations of Iterator no longer have to override 
remove if they desire the default behavior, which is to throw an 
UnsupportedOperationException.


http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which 
happens to implement both Iterable and Iterator. Now both Iterable and 
Iterator supply a default forEach method, so the compiler balks. One 
minimally intrusive solution is for this class to override both 
defaults and provide its own version of forEach.


http://cr.openjdk.java.net/~akhil/8005053.0/webrev/

Please review
Thanks

Re: RFR: 8005051: default methods for Iterator

[Remi Forax]

On 12/14/2012 02:00 PM, Paul Sandoz wrote:

On Dec 14, 2012, at 11:54 AM, Remi Forax  wrote:

We can't remove Collection.forEach without having perf issue because the stream 
pipeline use it.
Iterator.forEach can be removed but it's a pity because it's really convenient,

And a case can be made performance wise too (there are optimal implementations 
in the stream code base).

I have found that forEach, in general, has been very useful. Since it abstracts 
away the details of traversal it has enabled better re-use of code.


The VM profiles the iterator when calling hasNext and next, if you end 
up by using Iterator.forEach instead of one of its overriden methods, 
you share the profile between part of the pipeline that should not be 
shared. So using Iterator.forEach may be less performant or not 
depending if forEach is overriden by a specific implementation or not.




Paul.


Rémi




a possble solution is to rename Iterator.forEach() to something else.


Please review
Thanks


cheers,
Rémi

Re: RFR: 8005051: default methods for Iterator

[Paul Sandoz]

On Dec 14, 2012, at 11:54 AM, Remi Forax  wrote:
> 
> We can't remove Collection.forEach without having perf issue because the 
> stream pipeline use it.
> Iterator.forEach can be removed but it's a pity because it's really 
> convenient,

And a case can be made performance wise too (there are optimal implementations 
in the stream code base). 

I have found that forEach, in general, has been very useful. Since it abstracts 
away the details of traversal it has enabled better re-use of code.

Paul.

> a possble solution is to rename Iterator.forEach() to something else.
> 
>> 
>> Please review
>> Thanks
>> 
> 
> cheers,
> Rémi
> 

Re: RFR: 8005051: default methods for Iterator

[Lance Andersen - Oracle]

On Dec 14, 2012, at 7:28 AM, Alan Bateman wrote:

> On 14/12/2012 01:24, Akhil Arora wrote:
>> As part of the library lambdafication, this patch adds a forEach default 
>> method to Iterator, and converts remove() into a default method so that 
>> implementations of Iterator no longer have to override remove if they desire 
>> the default behavior, which is to throw an UnsupportedOperationException.
>> 
>> http://cr.openjdk.java.net/~akhil/8005051.0/webrev/
> I looked at the changes to Iterator, a few minor comments:
> 
> I think it would help to change "This default implementation" to "The default 
> implementation", it makes it more obviously normative (and so testable) and 
> might help for sub-types that override the method but don't inherit the 
> javadoc.
> 
> I assume the generated javadoc ends as a long paragraph, did you consider 
> putting in  tags to make it a bit easier on the reader, minimally put the 
> description of the default implementation isn't own paragraph.

This is why i am a proponent of adding a javadoc tag for default methods so 
that everyone is consistent in where/how we document the default method.  I 
worry if we do not developers will place this info in various places making it 
easy to overlook.
> 
> Should Collection have a lowercase "c" as it may be an iterator over a 
> collection that is not a java.util.Collection?
> 
> In forEach then it may be smoother to change "... execution subsequent ..." 
> to "... execution then subsequent ...".
> 
> As per the other thread, if new methods are coming with the public modifier 
> then I think it should be added to the existing methods.
> 
> -Alan.
> 
> 


Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8005051: default methods for Iterator

[Alan Bateman]

On 14/12/2012 01:24, Akhil Arora wrote:
As part of the library lambdafication, this patch adds a forEach 
default method to Iterator, and converts remove() into a default 
method so that implementations of Iterator no longer have to override 
remove if they desire the default behavior, which is to throw an 
UnsupportedOperationException.


http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

I looked at the changes to Iterator, a few minor comments:

I think it would help to change "This default implementation" to "The 
default implementation", it makes it more obviously normative (and so 
testable) and might help for sub-types that override the method but 
don't inherit the javadoc.


I assume the generated javadoc ends as a long paragraph, did you 
consider putting in  tags to make it a bit easier on the reader, 
minimally put the description of the default implementation isn't own 
paragraph.


Should Collection have a lowercase "c" as it may be an iterator over a 
collection that is not a java.util.Collection?


In forEach then it may be smoother to change "... execution subsequent 
..." to "... execution then subsequent ...".


As per the other thread, if new methods are coming with the public 
modifier then I think it should be added to the existing methods.


-Alan.

Re: RFR: 8005051: default methods for Iterator

[Chris Hegarty]

On 14/12/2012 01:24, Akhil Arora wrote:

As part of the library lambdafication, this patch adds a forEach default
method to Iterator, and converts remove() into a default method so that
implementations of Iterator no longer have to override remove if they
desire the default behavior, which is to throw an
UnsupportedOperationException.


This is not specified. Different Java SE implementations can have 
different default implementations of remove(). Your statement is either 
mistaken, or the changes will not meet their original intent.


forEach, it is not clear to me if the method description is specifying 
what the default implementation is supposed to do, or if it is 
specifying the contract for the method.


This, of course, has the same underlying cause as what is being 
discussed in other mail threads. I would like to see a clear decision 
being made around how default methods are to be specified before we 
start moving them into jdk8.


-Chris.



http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which
happens to implement both Iterable and Iterator. Now both Iterable and
Iterator supply a default forEach method, so the compiler balks. One
minimally intrusive solution is for this class to override both defaults
and provide its own version of forEach.

http://cr.openjdk.java.net/~akhil/8005053.0/webrev/

Please review
Thanks

Re: RFR: 8005051: default methods for Iterator

[Remi Forax]

Hi Akhil,

On 12/14/2012 02:24 AM, Akhil Arora wrote:

As part of the library lambdafication, this patch adds a forEach default
method to Iterator, and converts remove() into a default method so that
implementations of Iterator no longer have to override remove if they
desire the default behavior, which is to throw an
UnsupportedOperationException.

http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which
happens to implement both Iterable and Iterator. Now both Iterable and
Iterator supply a default forEach method, so the compiler balks. One
minimally intrusive solution is for this class to override both defaults
and provide its own version of forEach.

http://cr.openjdk.java.net/~akhil/8005053.0/webrev/


The issue here is in the code of ASM wich is an external library 
imported in the JDK,

so not sure it's a good idea to patch it.

Moreover, goggling for "implement Iterable, Iterator" shows that this 
anti-pattern is used too frequently to not step back and see if a better 
solution is not possible.

https://encrypted.google.com/search?hl=en&q=%22implements+Iterable%3C*%3E%2C+Iterator%3C*%3E%22

We can't remove Collection.forEach without having perf issue because the 
stream pipeline use it.
Iterator.forEach can be removed but it's a pity because it's really 
convenient,

a possble solution is to rename Iterator.forEach() to something else.



Please review
Thanks



cheers,
Rémi

RFR: 8005051: default methods for Iterator

[Akhil Arora]

As part of the library lambdafication, this patch adds a forEach default 
method to Iterator, and converts remove() into a default method so that 
implementations of Iterator no longer have to override remove if they 
desire the default behavior, which is to throw an 
UnsupportedOperationException.


http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which 
happens to implement both Iterable and Iterator. Now both Iterable and 
Iterator supply a default forEach method, so the compiler balks. One 
minimally intrusive solution is for this class to override both defaults 
and provide its own version of forEach.


http://cr.openjdk.java.net/~akhil/8005053.0/webrev/

Please review
Thanks