Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v3]
On Fri, 19 Mar 2021 01:17:34 GMT, Alexey Semenyuk wrote: >> Ian Graves has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains two commits: >> >> - Clarifying type argument and shorting a toArray invocation >> - Converting jpackage to use Stream.toList() > > Marked as reviewed by asemenyuk (Reviewer). Ready for sponsoring when you all are. - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v3]
On Tue, 16 Mar 2021 04:55:23 GMT, Ian Graves wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > Ian Graves has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains two commits: > > - Clarifying type argument and shorting a toArray invocation > - Converting jpackage to use Stream.toList() Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Thu, 18 Mar 2021 19:51:06 GMT, Ian Graves wrote: >> @igraves as a "best practice" try to avoid doing a force-push to a branch >> with an active code review. It makes it hard for reviewers to see what has >> changed. If you need to merge changes from master, use "git merge master" >> instead. >> >> @alexeysemenyukoracle @sashamatveev Can you re-review this so Ian can >> integrate it? > > @kevinrushforth Understood, thanks! Looks fine. - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Thu, 18 Mar 2021 19:43:10 GMT, Kevin Rushforth wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > @igraves as a "best practice" try to avoid doing a force-push to a branch > with an active code review. It makes it hard for reviewers to see what has > changed. If you need to merge changes from master, use "git merge master" > instead. > > @alexeysemenyukoracle @sashamatveev Can you re-review this so Ian can > integrate it? @kevinrushforth Understood, thanks! - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Sun, 14 Mar 2021 18:22:50 GMT, Ian Graves wrote: > This converts jpackage to use `Stream.toList()` instead of > `Stream.collect(Collectors.toList())`. One piece of code was modified to not > mutate a list in addition to one test that used a mutating sort on a list. > The rest of the changes are simple substitutions. @igraves as a "best practice" try to avoid doing a force-push to a branch with an active code review. It makes it hard for reviewers to see what has changed. If you need to merge changes from master, use "git merge master" instead. @alexeysemenyukoracle @sashamatveev Can you re-review this so Ian can integrate it? - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v3]
> This converts jpackage to use `Stream.toList()` instead of > `Stream.collect(Collectors.toList())`. One piece of code was modified to not > mutate a list in addition to one test that used a mutating sort on a list. > The rest of the changes are simple substitutions. Ian Graves has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Clarifying type argument and shorting a toArray invocation - Converting jpackage to use Stream.toList() - Changes: https://git.openjdk.java.net/jdk/pull/2997/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2997=02 Stats: 47 lines in 17 files changed: 1 ins; 3 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/2997.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2997/head:pull/2997 PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 18:53:26 GMT, Ian Graves wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Clarifying type argument and shorting a toArray invocation Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 18:53:26 GMT, Ian Graves wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Clarifying type argument and shorting a toArray invocation Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 17:29:46 GMT, Alexey Semenyuk wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clarifying type argument and shorting a toArray invocation
>
> src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java
> line 200:
>
>> 198: ).map(e -> {
>> 199: e.getValue().setOutputFileName(e.getKey());
>> 200: return (WixFragmentBuilder) e.getValue();
>
> Why this explicit cast is needed here?
The explicit cast is needed because `Stream.toList()` is invariant, but the
stream is producing a covariant type (by way of the type inferencer and
`Stream.of(..)` accepting two subclasses of `WixFragmentBuilder`). We need to
introduce a hint to bring the type back to the invariant form. On second
thought I'm going to make this explicit type argument instead so it's clear
that the casting isn't part to what the code is otherwise doing.
-
PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
> This converts jpackage to use `Stream.toList()` instead of > `Stream.collect(Collectors.toList())`. One piece of code was modified to not > mutate a list in addition to one test that used a mutating sort on a list. > The rest of the changes are simple substitutions. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Clarifying type argument and shorting a toArray invocation - Changes: - all: https://git.openjdk.java.net/jdk/pull/2997/files - new: https://git.openjdk.java.net/jdk/pull/2997/files/1bb46b54..ce11489b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2997=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2997=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2997.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2997/head:pull/2997 PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Mon, 15 Mar 2021 17:31:10 GMT, Alexey Semenyuk wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java line > 151: > >> 149: components.add(BigInteger.ZERO); >> 150: } >> 151: return components.stream().toList().toArray(BigInteger[]::new); > > I guess this can be simplified down to > `components.stream().toArray(BigInteger[]::new);` Good catch! - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Sun, 14 Mar 2021 18:22:50 GMT, Ian Graves wrote:
> This converts jpackage to use `Stream.toList()` instead of
> `Stream.collect(Collectors.toList())`. One piece of code was modified to not
> mutate a list in addition to one test that used a mutating sort on a list.
> The rest of the changes are simple substitutions.
Looks good. Minor improvements suggested.
src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java line
200:
> 198: ).map(e -> {
> 199: e.getValue().setOutputFileName(e.getKey());
> 200: return (WixFragmentBuilder) e.getValue();
Why this explicit cast is needed here?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java line
151:
> 149: components.add(BigInteger.ZERO);
> 150: }
> 151: return components.stream().toList().toArray(BigInteger[]::new);
I guess this can be simplified down to
`components.stream().toArray(BigInteger[]::new);`
-
PR: https://git.openjdk.java.net/jdk/pull/2997
