Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v3]
> Collections.sort is just a wrapper, so it is better to use an instance method > directly. Andrey Turbanov has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5229/files - new: https://git.openjdk.java.net/jdk/pull/5229/files/d6dfc8bf..e31936a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5229&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5229&range=01-02 Stats: 21 lines in 10 files changed: 4 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/5229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5229/head:pull/5229 PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v2]
On Wed, 25 Aug 2021 12:47:41 GMT, Andrey Turbanov wrote: >> src/java.base/share/classes/java/net/URLPermission.java line 222: >> >>> 220: >>> 221: List l = normalizeMethods(methods); >>> 222: l.sort(null); >> >> I am not opposed to this change, but I find this is slightly more ugly than >> `Collections.sort(l)`; so I have to ask: Is there any noticeable benefit? > > Actually I agree with you. > One more point is that List.sort(null) doesn't check at compile time that > class implements `Comparable`. But Collections.sort have this check. > I will revert last commit. > @azvegint are you ok with that? No objections. - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v2]
On Wed, 25 Aug 2021 08:29:57 GMT, Daniel Fuchs wrote: >> Andrey Turbanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8272863: Replace usages of Collections.sort with List.sort call in public >> java modules >> replace Collections.sort without comparator too > > src/java.base/share/classes/java/net/URLPermission.java line 222: > >> 220: >> 221: List l = normalizeMethods(methods); >> 222: l.sort(null); > > I am not opposed to this change, but I find this is slightly more ugly than > `Collections.sort(l)`; so I have to ask: Is there any noticeable benefit? Actually I agree with you. One more point is that List.sort(null) doesn't check at compile time that class implements `Comparable`. But Collections.sort have this check. I will revert last commit. @azvegint are you ok with that? - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v2]
On Tue, 24 Aug 2021 20:21:52 GMT, Andrey Turbanov wrote: >> Collections.sort is just a wrapper, so it is better to use an instance >> method directly. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8272863: Replace usages of Collections.sort with List.sort call in public > java modules > replace Collections.sort without comparator too src/java.base/share/classes/java/net/URLPermission.java line 222: > 220: > 221: List l = normalizeMethods(methods); > 222: l.sort(null); I am not opposed to this change, but I find this is slightly more ugly than `Collections.sort(l)`; so I have to ask: Is there any noticeable benefit? - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Tue, 24 Aug 2021 11:48:46 GMT, Alexander Zvegintsev wrote: > Is there any reason to not touch them along with this fix? Update them too. - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v2]
> Collections.sort is just a wrapper, so it is better to use an instance method > directly. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8272863: Replace usages of Collections.sort with List.sort call in public java modules replace Collections.sort without comparator too - Changes: - all: https://git.openjdk.java.net/jdk/pull/5229/files - new: https://git.openjdk.java.net/jdk/pull/5229/files/e31936a5..d6dfc8bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5229&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5229&range=00-01 Stats: 21 lines in 10 files changed: 0 ins; 4 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/5229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5229/head:pull/5229 PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. java.time changes look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. There are a bunch of calls to `Collections.sort()` without a comparator specified (at least in java.desktop): https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/java2d/Spans.java#L144 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/DebugSettings.java#L278 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/swing/text/TextComponentPrintable.java#L787 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/DefaultRowSorter.java#L1070 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/DefaultRowSorter.java#L1204 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/GroupLayout.java#L2137 It is also a wrapper to `list.sort(null)`. Is there any reason to not touch them along with this fix? - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. java/net and sun/net changes LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. The changes in the src/java.desktop/ looks fine. Filed: https://bugs.openjdk.java.net/browse/JDK-8272863 - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
