RFR: JDK-8273038: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel) [v4]

[Markus KARG]

On Sun, 29 Aug 2021 16:24:01 GMT, Alan Bateman <alanb at openjdk.org> wrote:

> The src/ implementation changes are fine, I think we agreed those previously.

Agreed.

> It would be cleaner if this test had been created as TestNG test. That would allow us to get rid of Thrower and the assertThrowsXXX infrastructure. It would allow us to use data providers too. Maybe it can be converted and cleaned up in a future PR.

I confirm that I will migrate to TestNG in a future PR.

> I think I mentioned this previously but the test can go into test/jdk/java/nio/channels/Channels so that it's in the same directory as the other tests for the Channels API/implementation.

Done in https://github.com/openjdk/jdk/pull/5209/commits/c886fbc26e18262539473bd66f091083fe9b7fbf.

> ifOutIsNullThenNpeIsThrown is a strange method name, can we rename it to something testNullPointerException or something more obvious?

Done in https://github.com/openjdk/jdk/pull/5209/commits/90a176f346d65782b703f194197c6858bce1077b.

> "contents" is also a bit strange for the method name. This where the real test is so let's rename that to something more obvious too.

Done in https://github.com/openjdk/jdk/pull/5209/commits/6b4c34c707778c68d3b06cc401c8c175918a60ea.

>There's a typo in one of the comments here it says "starting point", should be "starting position".

Done in https://github.com/openjdk/jdk/pull/5209/commits/cb2a863665f3fb329bd9c9237fc634a046d2edf1.

> You've got reasonable coverage for random initial positions in both source and target files, good! One other case that could be tested is where the initial position is beyond EOF. If the initial position of the source file is beyond EOF then 0 bytes will be transferred. If the initial position of the target file is beyond EOF then it will be extended.

Done in https://github.com/openjdk/jdk/pull/5209/commits/852032d9bbe102ebec1b03de21492221003180cd.

>At some point we should look at adding a test for files > 2Gb as otherwise we aren't testing the loop (as we discussed).

Agreed that *at some point* (i. e. in *some* future PR) someone could provide such a test. As explained in https://github.com/openjdk/jdk/pull/5209#issuecomment-907782217 there is no 100% path coverage rule in OpenJDK, so while this *might* be a good idea, it seems it is definitively not a *mandatory* constraint to test that loop at all.

> A few minor nits: The copyright date says "2014", I assume copied from another test.

It actually reads "2014, 2021" which means that the original code was first published in 2014 and was last modified in 2021. Oracle once asked me to always do it *exactly that way* when touching any Oracle source code (which I did here, as the test originated from existing Oracle source code). If OpenJDK is an exceptional case here, then please confirm that I am *officially allowed* to remove the year 2014 from that list. Until then I have to keep the line exactly the way it is to not get into any troubles with Oracle's legal department.

> There are several unused imports to remove.

Done in https://github.com/openjdk/jdk/pull/5209/commits/c3dbc33880ebea4b11f014c6c0046f3d5cc24e1e.

>The static modifiers on interfaces aren't needed.

Done in https://github.com/openjdk/jdk/pull/5209/commits/f3a6551d009d2a22d6d2aa448025330623c64065.

>There are few really long lines, one is 150+ that wraps and will be annoying for side-by-side diffs when changed in the future.

Done in https://github.com/openjdk/jdk/pull/5209/commits/412fab109c94de21a19cdc6a8aa3060e41deeb51. The longest code line now is 115 characters which should be fine for side-by-side views as my screen shows more than 250 characters per line.

I hope that with my recent changes this PR now is in a shape which allows it to get merged. As we agreed on multiple small PRs instead of a single big one, our target should be to contain only the *essential changes per each PR* but add *more PRs ontop* for non-essential improvements (which I will be happy to provide).

-------------

PR: https://git.openjdk.java.net/jdk/pull/5209