Re: Doc updates was Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
The docs changes look good to me. Mike On Jan 13 2014, at 05:39 , Paul Sandoz paul.san...@oracle.com wrote: On Jan 10, 2014, at 3:01 PM, Alan Bateman alan.bate...@oracle.com wrote: The implementation changes look good. I agree that the javadoc needs changing as it's otherwise misleading as to what the implementation actually does. I would think that this should go with the implementation change rather than as a separate change. See here for patch layered on top of code changes (to be folded after review into one patch): http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort-docs/webrev/ Paul.
Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
On Jan 10, 2014, at 6:56 PM, Mike Duigou mike.dui...@oracle.com wrote: The implementation looks good. I would move construction of the listIterator to before the toArray() for detection of concurrent modification (growing of the list). If there is another thread concurrently operating on a non-current list during invocation of the sort method then it's gonna produce weird results, and any concurrent-like list should be overriding this impl and locking the sort (e.g. COWAL). For single threaded execution i suppose the Comparator could monkey around with the list, it's a long shot but is that what you are getting at? Paul. I believe we should fix this for 8 if possible. Mike
Doc updates was Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
On Jan 10, 2014, at 3:01 PM, Alan Bateman alan.bate...@oracle.com wrote: The implementation changes look good. I agree that the javadoc needs changing as it's otherwise misleading as to what the implementation actually does. I would think that this should go with the implementation change rather than as a separate change. See here for patch layered on top of code changes (to be folded after review into one patch): http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort-docs/webrev/ Paul.
RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul.
Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
On 10/01/2014 11:21, Paul Sandoz wrote: Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul. The implementation changes look good. I agree that the javadoc needs changing as it's otherwise misleading as to what the implementation actually does. I would think that this should go with the implementation change rather than as a separate change. So is the stack overflow concern with List implementations that were originally developed to target JDK 7 or older? In any case, this is the type of change more suitable to a major release. -Alan.
Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
The implementation looks good. I would move construction of the listIterator to before the toArray() for detection of concurrent modification (growing of the list). I believe we should fix this for 8 if possible. Mike On Jan 10 2014, at 03:21 , Paul Sandoz paul.san...@oracle.com wrote: Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul.
