RFR: 8303260: (fc) FileChannel::transferFrom should support position > size() [v4]

[Alan Bateman]

On Wed, 29 Mar 2023 00:01:22 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Enhance `FileChannel::transferFrom` to extend the channel's file if `position > size()`. This is consistent with `FileChannel::write(ByteBuffer, long)`.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8303260: Increment copyright year

src/java.base/share/classes/java/nio/channels/FileChannel.java line 815:

> 813:      * file will be grown to accommodate the new bytes; the values of any bytes
> 814:      * between the previous end-of-file and the newly-written bytes are
> 815:      * unspecified.  </p>

One thing that I missed in the initial round is that the description of parameter "position" says "within the file". That is no longer correct, it should say "The position within the file at which the transfer is to begin".

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 26:

> 24: /* @test
> 25:  * @bug 8303260
> 26:  * @summary Test transferFrom to a position greater than size

Might be clearer to say "file size" as it might be read as the transfer size.

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 96:

> 94:     // This method therefore tests the direct path on Linux and the mapped
> 95:     // path on other platforms.
> 96:     //

Would be possible to change the method description to /* .. */. Also I wonder if we can trim down this comment as it will go stale very quickly due to churn in the implementation. Maybe reduce it down to say that it tests transfer from a channel type that may be optimized?

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 99:

> 97:     @ParameterizedTest
> 98:     @MethodSource("fastParamProvider")
> 99:     void fromFast(long initialPosition, int bufSize, long offset) throws IOException {

I think it would be clearer to rename this fromFileChannel.

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 123:

> 121:     @MethodSource("readingByteChannelParamProvider")
> 122:     void fromReadingByteChannel(long initialPosition, int bufSize, long offset)
> 123:         throws IOException {

Indent issue here too.

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 141:

> 139:      * @param bytes      the bytes expected to be transferred
> 140:      */
> 141:     private static void transferFrom(ReadableByteChannel src, long count,

What would you think about renaming this method to testTransferFrom as that makes it clearer at the use-sites.

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 143:

> 141:     private static void transferFrom(ReadableByteChannel src, long count,
> 142:                                      long initialPos, long offset, byte[] bytes)
> 143:         throws IOException {

Can you fix the indention here, it's hard to see where the method declaration ends and the body starts.

test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 153:

> 151:             long transferred = target.transferFrom(src, position, count);
> 152:             assertTrue(transferred >= 0, "transferFrom returned negative");
> 153:             assertFalse(transferred > count, transferred + " > " + count);

It might be a bit clearer to assert that transferred < count.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155265519
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155263740
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155264199
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155266483
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155265756
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155265838
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155265004
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1155265649