Skip to content

8361299: (bf) CharBuffer.getChars(int,int,char[],int) violates pre-existing specification #26104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions src/java.base/share/classes/java/nio/X-Buffer.java.template
Original file line number Diff line number Diff line change
Expand Up @@ -1897,21 +1897,31 @@ public abstract sealed class $Type$Buffer
#if[char]

/**
* Absolute bulk <i>get</i> method.
* Relative bulk <i>get</i> method.
*
* <p> This method transfers {@code srcEnd-srcBegin} characters from this
* buffer into the given array, starting at index {@code srcBegin} in this
* buffer and at offset {@code dstBegin} in the array. The position of this
* buffer is unchanged.
* buffer into the given array, starting at index
* {@code position() + srcBegin} in this buffer and at offset
* {@code dstBegin} in the array. The position of this buffer is unchanged.
*
* <p> An invocation of this method behaves exactly the same was as the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use an implSpec for this and keep "This method is equivalent to".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use an implSpec for this and keep "This method is equivalent to".

I was following the existing convention elsewhere in the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why JDK-8343110 put the implSpec after the param/return/throws tags so happy it has been fixed. Maybe at some point we can use implSpec to these methods.

* invocation
*
* {@snippet lang=java :
* get(position() + srcBegin, dst, dstBegin, srcEnd - srcBegin)
* }
*
* @param srcBegin
* The index in this buffer from which the first character will be
* read; must be non-negative and less than {@code limit()}
* The index in this buffer, relative to the current position,
* of the first character to
* read; must be non-negative and less than
* {@code limit() - position()}
*
* @param srcEnd
* The index in this buffer directly before the last character to
* read; must be non-negative and less or equal than {@code limit()}
* and must be greater or equal than {@code srcBegin}
* The index in this buffer, relative to the current position,
* after the last character to read;
* must be greater than or equal to {@code srcBegin} and less than
* or equal to {@code limit() - position()}
*
* @param dst
* The destination array
Expand All @@ -1924,14 +1934,16 @@ public abstract sealed class $Type$Buffer
* If the preconditions on the {@code srcBegin}, {@code srcEnd},
* and {@code dstBegin} parameters do not hold
*
* @implSpec This method is equivalent to
* {@code get(srcBegin, dst, dstBegin, srcEnd - srcBegin)}.
*
* @since 25
*/
@Override
public void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) {
get(srcBegin, dst, dstBegin, srcEnd - srcBegin);
// Check [srcBegin,srcEnd) is a subset of [0,limit()-position)
int pos = position();
int lim = limit();
Objects.checkFromToIndex(srcBegin, srcEnd, lim - pos);

get(pos + srcBegin, dst, dstBegin, srcEnd - srcBegin);
}

/**
Expand Down
17 changes: 15 additions & 2 deletions test/jdk/java/nio/Buffer/GetChars.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

/**
* @test
* @bug 8343110
* @bug 8343110 8361299
* @summary Check for expected behavior of CharBuffer.getChars().
* @run testng GetChars
* @key randomness
Expand Down Expand Up @@ -71,6 +71,19 @@ public void testSrcBeginIsNegative() {
() -> CB.getChars(-1, 3, new char[4], 0));
}

@Test
public void testSrcBeginIsNegationOfPosition() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed through GetChars and I see it only tests a CB created with CharBuffer.wrap, only testGetChars tests all char buffers. All the test methods should be tested all char buffer implementations. So maybe we can create a follow-up issue to improve this test (and probably migrate it to a JUnit test too).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe we can create a follow-up issue to improve this test (and probably migrate it to a JUnit test too).

Agreed. I also noticed it is TestNG now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's create a follow-up issue for that.

CB.position(1);
Assert.assertThrows(IndexOutOfBoundsException.class,
() -> {
try {
CB.getChars(-1, 3, new char[4], 0);
} finally {
CB.position(0);
}
});
}

@Test
public void testDstBeginIsNegative() {
Assert.assertThrows(IndexOutOfBoundsException.class,
Expand Down Expand Up @@ -200,7 +213,7 @@ public void testGetChars(String type, CharBuffer cb) {
System.out.format("%s position=%d, limit=%d%n", type, cb.position(), cb.limit());
int expected = intSum(cb);
var dst = new char[cb.remaining()];
cb.getChars(cb.position(), cb.limit(), dst, 0);
cb.getChars(0, cb.remaining(), dst, 0);
int actual = intSum(dst);
assertEquals(actual, expected);
}
Expand Down