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

[Brian Burkhalter]

On Sun, 19 Mar 2023 07:30:09 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8303260: Convert test to JUnit 5
>
> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 98:
> 
>> 96:     @MethodSource("fastParamProvider")
>> 97:     void fromFast(long initialPosition, int bufSize, long offset)
>> 98:         throws IOException {
> 
> Aligning the "throws IOException {" with the method body makes it hard to read, can you move this to the previous line?

Changed in 085746fa9e7446c380ed08c951e52796915c1f53.

> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 104:
> 
>> 102:             byte[] bytes = new byte[bufSize];
>> 103:             RND.nextBytes(bytes);
>> 104:             src.write(ByteBuffer.wrap(bytes), 0);
> 
> Return value from write probably needs to be checked as a short write is possible in some cases.

Changed in 085746fa9e7446c380ed08c951e52796915c1f53.

> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 115:
> 
>> 113:     @ParameterizedTest
>> 114:     @MethodSource("slowParamProvider")
>> 115:     void fromSlow(long initialPosition, int bufSize, long offset)
> 
> A suggestion here is to rename this to fromReadingByteChannel to make a bit clearer.

Changed in 085746fa9e7446c380ed08c951e52796915c1f53.

> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 135:
> 
>> 133:      *
>> 134:      * @throws RuntimeException if an unexpected number of bytes is transferred
>> 135:      *                          or the transferred values are not as expected
> 
> This is a JUnit tests so I assume it should use the assertXXX methods rather than throwing RuntimeException.

Changed in 085746fa9e7446c380ed08c951e52796915c1f53.

> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 144:
> 
>> 142:                                                    READ, WRITE)) {
>> 143:             target.position(initialPos);
>> 144:             target.write(ByteBuffer.wrap(new byte[] {(byte)42}));
> 
> I assume you should assertEquals that write return 1 here.

So changed in 085746fa9e7446c380ed08c951e52796915c1f53.

> test/jdk/java/nio/channels/FileChannel/TransferFromExtend.java line 149:
> 
>> 147:             long transferred = target.transferFrom(src, position, count);
>> 148:             if (transferred != count)
>> 149:                 throw new RuntimeException(transferred + " != " + count);
> 
> transferFrom is allowed to return a result < count. There are cases where this can happen in the JDK implementation so I think you just want to check that transferred > 0 and that compare that portion of the file, right?

Changed in 085746fa9e7446c380ed08c951e52796915c1f53.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142409486
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142409943
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142409776
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142408864
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142408599
PR Review Comment: https://git.openjdk.org/jdk/pull/12797#discussion_r1142409228