Re: RFR: 8005051: default methods for Iterator
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=enq=%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
Re: RFR: 8005051: default methods for Iterator
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
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 p 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
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 p 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
On Dec 14, 2012, at 11:54 AM, Remi Forax fo...@univ-mlv.fr 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
On 12/14/2012 02:00 PM, Paul Sandoz wrote: On Dec 14, 2012, at 11:54 AM, Remi Forax fo...@univ-mlv.fr 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
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 ListT 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 peter.lev...@gmail.com 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/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/http://cr.openjdk.java.net/~akhil/8005053.0/webrev/ Please review Thanks
Re: RFR: 8005051: default methods for Iterator
On Fri, Dec 14, 2012 at 4:54 AM, Remi Forax fo...@univ-mlv.fr 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=enq=%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
