Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v3]
> Hi, > > Could someone please review my code for updating the code in the `java.net` > and `java.nio` packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - 8268056: reverted changes to FileTime - Merge remote-tracking branch 'origin/master' into JDK-8268056 - 8268056: Reverted changes to URLDecoder; reformatted change to FileTime - 8268056: Update java.net and java.nio to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4285/files - new: https://git.openjdk.java.net/jdk/pull/4285/files/2f179b52..23f53c52 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=01-02 Stats: 3191 lines in 172 files changed: 1661 ins; 1173 del; 357 mod Patch: https://git.openjdk.java.net/jdk/pull/4285.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4285/head:pull/4285 PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v3]
On Tue, 1 Jun 2021 17:46:10 GMT, Alan Bateman wrote: >> Sorry about that. I've changed it now. See 2f179b5 > > This still looks a bit messy because you've got 3 different styles in the one > switch statement. It's okay to drop FileTime from the patch if you want as > it's not worth spending time on. OK, if you prefer. I've reverted the changes now. See commit 23f53c5 - PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v3]
On Tue, 1 Jun 2021 16:58:12 GMT, Daniel Fuchs wrote: >> Patrick Concannon has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - 8268056: reverted changes to FileTime >> - Merge remote-tracking branch 'origin/master' into JDK-8268056 >> - 8268056: Reverted changes to URLDecoder; reformatted change to FileTime >> - 8268056: Update java.net and java.nio to use switch expressions > > src/java.base/share/classes/java/nio/file/attribute/FileTime.java line 240: > >> 238: Long.MAX_VALUE / SECONDS_PER_HOUR); >> 239: case MINUTES -> secs = scale(value, SECONDS_PER_MINUTE, >> 240: Long.MAX_VALUE / SECONDS_PER_MINUTE); > > It would be nicer to keep the second line aligned with the opening > parenthesis, as it was before. Change made in commit 2f179b5, but reverted all changes made to file as per Alan's request. See 23f53c5 - PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v2]
On Tue, 1 Jun 2021 17:20:38 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.net`
>> and `java.nio` packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8268056: Reverted changes to URLDecoder; reformatted change to FileTime
src/java.base/share/classes/java/nio/file/Files.java line 2832:
> 2830: result = FileVisitResult.CONTINUE;
> 2831: }
> 2832: default -> throw new AssertionError("Should not get
> here");
This is subjective, and we're still finding our way with how best to construct
some of the more complex switch expressions.
Where possible, I think it is best to remove assignments from the individual
case branches. The use of `yield` makes it very clear what is going on, and
ensures that each case branch, well... yields something. So how about:
FileVisitResult result = switch (ev.type()) {
case ENTRY -> {
IOException ioe = ev.ioeException();
if (ioe == null) {
assert ev.attributes() != null;
yield visitor.visitFile(ev.file(), ev.attributes());
} else {
yield visitor.visitFileFailed(ev.file(), ioe);
}
}
case START_DIRECTORY -> {
var r = visitor.preVisitDirectory(ev.file(), ev.attributes());
// if SKIP_SIBLINGS and SKIP_SUBTREE is returned then
// there shouldn't be any more events for the current
// directory.
if (r == FileVisitResult.SKIP_SUBTREE ||
r == FileVisitResult.SKIP_SIBLINGS)
walker.pop();
yield r;
}
case END_DIRECTORY -> {
var r = visitor.postVisitDirectory(ev.file(), ev.ioeException());
// SKIP_SIBLINGS is a no-op for postVisitDirectory
if (r == FileVisitResult.SKIP_SIBLINGS)
r = FileVisitResult.CONTINUE;
yield r;
}
default -> throw new AssertionError("Should not get here");
};
-
PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v4]
> Hi, > > Could someone please review my code for updating the code in the `java.net` > and `java.nio` packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8268056: Added yield to switch expression in Files - Changes: - all: https://git.openjdk.java.net/jdk/pull/4285/files - new: https://git.openjdk.java.net/jdk/pull/4285/files/23f53c52..433b02a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=02-03 Stats: 13 lines in 1 file changed: 2 ins; 1 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4285.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4285/head:pull/4285 PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v2]
On Wed, 2 Jun 2021 09:13:44 GMT, Chris Hegarty wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268056: Reverted changes to URLDecoder; reformatted change to FileTime
>
> src/java.base/share/classes/java/nio/file/Files.java line 2832:
>
>> 2830: result = FileVisitResult.CONTINUE;
>> 2831: }
>> 2832: default -> throw new AssertionError("Should not
>> get here");
>
> This is subjective, and we're still finding our way with how best to
> construct some of the more complex switch expressions.
>
> Where possible, I think it is best to remove assignments from the individual
> case branches. The use of `yield` makes it very clear what is going on, and
> ensures that each case branch, well... yields something. So how about:
>
>
> FileVisitResult result = switch (ev.type()) {
> case ENTRY -> {
> IOException ioe = ev.ioeException();
> if (ioe == null) {
> assert ev.attributes() != null;
> yield visitor.visitFile(ev.file(), ev.attributes());
> } else {
> yield visitor.visitFileFailed(ev.file(), ioe);
> }
> }
> case START_DIRECTORY -> {
> var r = visitor.preVisitDirectory(ev.file(), ev.attributes());
>
> // if SKIP_SIBLINGS and SKIP_SUBTREE is returned then
> // there shouldn't be any more events for the current
> // directory.
> if (r == FileVisitResult.SKIP_SUBTREE ||
> r == FileVisitResult.SKIP_SIBLINGS)
> walker.pop();
> yield r;
> }
> case END_DIRECTORY -> {
> var r = visitor.postVisitDirectory(ev.file(), ev.ioeException());
> // SKIP_SIBLINGS is a no-op for postVisitDirectory
> if (r == FileVisitResult.SKIP_SIBLINGS)
> r = FileVisitResult.CONTINUE;
> yield r;
> }
> default -> throw new AssertionError("Should not get here");
> };
OK. I've made that change now. See 433b02a
-
PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v4]
On Wed, 2 Jun 2021 11:06:46 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.net` >> and `java.nio` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268056: Added yield to switch expression in Files LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v4]
On Wed, 2 Jun 2021 11:06:46 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.net` >> and `java.nio` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268056: Added yield to switch expression in Files Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v9]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - copyright years - merge from master, resolve one conflict - Merge branch 'master' - merge from master - rename setSecurityManagerDirect to implSetSecurityManager - default behavior reverted to allow - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - ... and 4 more: https://git.openjdk.java.net/jdk/compare/19450b99...331389b5 - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=08 Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Integrated: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. This pull request has now been integrated. Changeset: 6765f902 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/6765f902505fbdd02f25b599f942437cd805cad1 Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Co-authored-by: Sean Mullan Co-authored-by: Lance Andersen Co-authored-by: Weijun Wang Reviewed-by: erikj, darcy, chegar, naoto, joehw, alanb, mchung, kcr, prr, lancea - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value if not void. The local variable will be called `tmp`
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>>
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update FtpClient.java
The dependent pull request has now been integrated, and the target branch of
this pull request has been updated. This means that changes from the dependent
pull request can start to show up as belonging to this pull request, which may
be confusing for reviewers. To remedy this situation, simply merge the latest
changes from the new target branch into this pull request by running commands
similar to these in the local repository for your personal fork:
git checkout 8266459
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Withdrawn: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > The usage of `LinkedList` is senseless and can be replaced with either > `ArrayList` or `ArrayDeque` which are both more compact and effective. > > jdk:tier1 and jdk:tier2 are both ok This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/2744
RFR: 8263561: Re-examine uses of LinkedList
After I've renamed remove branch GitHub for some reason has closed original https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. - Commit messages: - Merge branch 'master' into 8263561 - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4304&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263561 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v4]
On Wed, 2 Jun 2021 11:06:46 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.net` >> and `java.nio` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268056: Added yield to switch expression in Files Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v5]
> Hi, > > Could someone please review my code for updating the code in the `java.net` > and `java.nio` packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8268056 - 8268056: Added yield to switch expression in Files - 8268056: reverted changes to FileTime - Merge remote-tracking branch 'origin/master' into JDK-8268056 - 8268056: Reverted changes to URLDecoder; reformatted change to FileTime - 8268056: Update java.net and java.nio to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4285/files - new: https://git.openjdk.java.net/jdk/pull/4285/files/433b02a0..0f7614e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4285&range=03-04 Stats: 17957 lines in 1083 files changed: 11155 ins; 3991 del; 2811 mod Patch: https://git.openjdk.java.net/jdk/pull/4285.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4285/head:pull/4285 PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v4]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value if not void. The local variable will be called `tmp` if
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
>
> I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains one commit:
8267521: Post JEP 411 refactoring: maximum covering > 50K
-
Changes: https://git.openjdk.java.net/jdk/pull/4138/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=03
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Integrated: 8267521: Post JEP 411 refactoring: maximum covering > 50K
On Fri, 21 May 2021 01:52:27 GMT, Weijun Wang wrote:
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value if not void. The local variable will be called `tmp` if
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
>
> I'll add a copyright year update commit before integration.
This pull request has now been integrated.
Changeset: 508cec75
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/508cec7535cd0ad015d566389bc9e5f53ce4103b
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
8267521: Post JEP 411 refactoring: maximum covering > 50K
Reviewed-by: dfuchs, prr
-
PR: https://git.openjdk.java.net/jdk/pull/4138
RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently
Please find below a fix that will make `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to the rare case where addresses bound to a network interfaces are updated while the test is running. Instead of using `NetworkInterface::equals` to compare network interfaces, the test now use `NetworkConfiguration::isSameInterface` which only looks at the network interface name and index and ignore the addresses which are bound to it. - Commit messages: - 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently Changes: https://git.openjdk.java.net/jdk/pull/4313/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4313&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264975 Stats: 36 lines in 3 files changed: 32 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4313/head:pull/4313 PR: https://git.openjdk.java.net/jdk/pull/4313
Re: RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently
On Wed, 2 Jun 2021 15:54:26 GMT, Daniel Fuchs wrote: > Please find below a fix that will make > `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to > the rare case where addresses bound to a network interfaces are updated while > the test is running. > > Instead of using `NetworkInterface::equals` to compare network interfaces, > the test now use `NetworkConfiguration::isSameInterface` which only looks at > the network interface name and index and ignore the addresses which are bound > to it. I think this looks okay although returning false if ni1 or ni2 is subtle (it follows the Objects.equals check so it's okay but I did have to look at it twice). - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4313
RFR: JDK-8268133 : Update java/net/Authenticator tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs
…ency on sun.net.www.MessageHeader and - Commit messages: - JDK-8268133 : Update java/net/Authenticator tests to eliminate dependency on sun.net.www.MessageHeader and Changes: https://git.openjdk.java.net/jdk/pull/4317/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4317&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268133 Stats: 321 lines in 6 files changed: 133 ins; 19 del; 169 mod Patch: https://git.openjdk.java.net/jdk/pull/4317.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4317/head:pull/4317 PR: https://git.openjdk.java.net/jdk/pull/4317
Re: RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently [v2]
On Wed, 2 Jun 2021 17:00:26 GMT, Alan Bateman wrote: > I think this looks okay although returning false if ni1 or ni2 is subtle (it > follows the Objects.equals check so it's okay but I did have to look at it > twice). Thanks Alan. I have added a comment to make it clearer. - PR: https://git.openjdk.java.net/jdk/pull/4313
Re: RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently [v2]
> Please find below a fix that will make > `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to > the rare case where addresses bound to a network interfaces are updated while > the test is running. > > Instead of using `NetworkInterface::equals` to compare network interfaces, > the test now use `NetworkConfiguration::isSameInterface` which only looks at > the network interface name and index and ignore the addresses which are bound > to it. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Add a comment for the non-obvious case where ni1 == null || ni2 == null - Changes: - all: https://git.openjdk.java.net/jdk/pull/4313/files - new: https://git.openjdk.java.net/jdk/pull/4313/files/aa5dea6b..d090333b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4313&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4313&range=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4313/head:pull/4313 PR: https://git.openjdk.java.net/jdk/pull/4313
Re: RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently [v2]
On Wed, 2 Jun 2021 17:52:34 GMT, Daniel Fuchs wrote: >> Please find below a fix that will make >> `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to >> the rare case where addresses bound to a network interfaces are updated >> while the test is running. >> >> Instead of using `NetworkInterface::equals` to compare network interfaces, >> the test now use `NetworkConfiguration::isSameInterface` which only looks at >> the network interface name and index and ignore the addresses which are >> bound to it. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment for the non-obvious case where ni1 == null || ni2 == null Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4313
Re: RFR: JDK-8268133 : Update java/net/Authenticator tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs
On Wed, 2 Jun 2021 17:22:01 GMT, Mahendra Chhipa
wrote:
> …ency on sun.net.www.MessageHeader and
test/jdk/java/net/Authenticator/B4722333.java line 79:
> 77: req.getResponseHeaders().set("Connection",
> "close");
> 78: req.getResponseHeaders().add("WWW-Authenticate",
> "Basic realm=\"foobar\"");
> 79: req.getResponseHeaders().add("WWW-Authenticate",
> "Foo realm=\"bar\"");
I believe this will change what the test is testing. I suggest replacing these
two lines with:
req.getResponseHeaders().add("WWW-Authenticate", "Basic realm="foobar" Foo
realm="bar"");
as in the original version of the test.
test/jdk/java/net/Authenticator/B4722333.java line 84:
> 82: case 4:
> 83: req.getResponseHeaders().set("Connection",
> "close");
> 84: req.getResponseHeaders().set("WWW-Authenticate",
> "Digest realm=\"biz\" domain=/foo nonce=\"thisisnonce \"");
I'm not very comfortable with changing the logic of the test. Can we keep the
strings identical please?
test/jdk/java/net/Authenticator/B4722333.java line 91:
> 89: req.getResponseHeaders().set("Connection",
> "close");
> 90: req.getResponseHeaders().set("WWW-Authenticate",
> "Digest realm=\"bizbar\" domain=/biz nonce=\"hereisanonce\"");
> 91: req.getResponseHeaders().add("WWW-Authenticate",
> "Basic realm=\"foobar\" Foo realm=\"bar\"");
Same remark here. Changing one WWW-Authenticate line into two WWW-Authenticate
lines may not trigger the same code path in the client.
test/jdk/java/net/Authenticator/B4722333.java line 98:
> 96: req.getResponseHeaders().set("WWW-Authenticate",
> "Foo p1=1 p2=2 p3=3 p4=4 p5=5 p6=6 p7=7 p8=8 p9=10");
> 97: req.getResponseHeaders().add("WWW-Authenticate",
> "Digest realm=foobiz domain=/foobiz nonce=newnonce");
> 98: req.getResponseHeaders().add("WWW-Authenticate",
> "Basic realm=bizbar");
Here again. Please keep the strings identical.
-
PR: https://git.openjdk.java.net/jdk/pull/4317
Re: RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently [v2]
On Wed, 2 Jun 2021 17:52:34 GMT, Daniel Fuchs wrote: >> Please find below a fix that will make >> `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to >> the rare case where addresses bound to a network interfaces are updated >> while the test is running. >> >> Instead of using `NetworkInterface::equals` to compare network interfaces, >> the test now use `NetworkConfiguration::isSameInterface` which only looks at >> the network interface name and index and ignore the addresses which are >> bound to it. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment for the non-obvious case where ni1 == null || ni2 == null Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4313
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v5]
On Wed, 2 Jun 2021 15:23:01 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.net` >> and `java.nio` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8268056 > - 8268056: Added yield to switch expression in Files > - 8268056: reverted changes to FileTime > - Merge remote-tracking branch 'origin/master' into JDK-8268056 > - 8268056: Reverted changes to URLDecoder; reformatted change to FileTime > - 8268056: Update java.net and java.nio to use switch expressions Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4285
Re: RFR: 8268056: Update java.net and java.nio to use switch expressions [v5]
On Wed, 2 Jun 2021 15:23:01 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.net`
>> and `java.nio` packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/master' into JDK-8268056
> - 8268056: Added yield to switch expression in Files
> - 8268056: reverted changes to FileTime
> - Merge remote-tracking branch 'origin/master' into JDK-8268056
> - 8268056: Reverted changes to URLDecoder; reformatted change to FileTime
> - 8268056: Update java.net and java.nio to use switch expressions
src/java.base/share/classes/java/nio/file/Files.java line 2817:
> 2815: }
> 2816: case START_DIRECTORY -> {
> 2817: var r = visitor.preVisitDirectory(ev.file(),
> ev.attributes());
Can you all uses of "r" with "res" to make it little bit clear that it's a
result, that will get it a bit more consistent with the exist code? Otherwise I
think this version is okay.
-
PR: https://git.openjdk.java.net/jdk/pull/4285
