RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs. CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912) ------------- Commit messages: - byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException Changes: https://git.openjdk.java.net/jdk/pull/2124/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2124&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259911 Stats: 8 lines in 2 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2124.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2124/head:pull/2124 PR: https://git.openjdk.java.net/jdk/pull/2124
On Mon, 18 Jan 2021 12:09:23 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs.
CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912)
The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is? ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
On Tue, 19 Jan 2021 19:42:38 GMT, Mandy Chung <mchung@openjdk.org> wrote:
The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is?
What would be the benefits? AFAICT the CSR is still needed since it's a small behavioral change, and updating the spec helps communicate that change. ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
On 1/19/21 12:33 PM, Claes Redestad wrote:
On Tue, 19 Jan 2021 19:42:38 GMT, Mandy Chung <mchung@openjdk.org> wrote:
The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is? What would be the benefits? AFAICT the CSR is still needed since it's a small behavioral change, and updating the spec helps communicate that change.
There is no behavioral change since AIIOBE is a subtype of OOBE. No spec change and so no CSR is needed. Mandy
On Tue, 19 Jan 2021 20:30:46 GMT, Claes Redestad <redestad@openjdk.org> wrote:
The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is?
The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is?
What would be the benefits? AFAICT the CSR is still needed since it's a small behavioral change, and updating the spec helps communicate that change.
I agree the change is allowable within the current spec. The behavior change is observable, though, and however unlikely there might be code that relies on exact class of the exception being thrown. So while perhaps not strictly needed, the spec change makes sense coming from the other way: If I have some array-based code then that will be throwing AIOOBE on OOBs. But the `byteArrayViewVarHandle` code is specified to throw IOOBE, so a developer picking it up would have to catch and rewrap exceptions or accept the compatibility risk. Harmonizing to specify AIOOBE improves the migration story. Case in point is #1855 which require changes to some tests that are expecting AIOOBE. ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
On Tue, 19 Jan 2021 20:59:24 GMT, Claes Redestad <redestad@openjdk.org> wrote:
I agree the change is allowable within the current spec. The behavior change is observable, though, and however unlikely there might be code that relies on exact class of the exception being thrown. So while perhaps not strictly needed, the spec change makes sense coming from the other way: If I have some array-based code then that will be throwing AIOOBE on OOBs. But the `byteArrayViewVarHandle` code is specified to throw IOOBE, so a developer picking it up would have to catch and rewrap exceptions or accept the compatibility risk. Harmonizing to specify AIOOBE improves the migration story.
I don't think there is any migration story. The exception type is an improved clarity. The spec change is very minor and I'm okay with or without the spec change.
Case in point is #1855 which require changes to some tests that are expecting AIOOBE.
Yes, that's what I was about to ask too. ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
On Mon, 18 Jan 2021 12:09:23 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs.
CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912)
Marked as reviewed by jvernee (Committer). src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4374:
4372: * <p> 4373: * Access of bytes at a given index will result in an 4374: * {@code ArrayIndexOutOfBoundsException} if the index is less than {@code 0}
Should the copyright year of this file also be updated? ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs.
CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912)
Claes Redestad 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: - Update VarHandles tests to check for AIOOBE specifically when appropriate - Copyrights - Merge branch 'master' into vh_aioobe - byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2124/files - new: https://git.openjdk.java.net/jdk/pull/2124/files/42c9460c..9259ee3a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2124&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2124&range=00-01 Stats: 4494 lines in 166 files changed: 725 ins; 2890 del; 879 mod Patch: https://git.openjdk.java.net/jdk/pull/2124.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2124/head:pull/2124 PR: https://git.openjdk.java.net/jdk/pull/2124
On Wed, 20 Jan 2021 18:47:18 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs.
CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912)
Claes Redestad 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:
- Update VarHandles tests to check for AIOOBE specifically when appropriate - Copyrights - Merge branch 'master' into vh_aioobe - byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
Looks good. Thanks for updating the test. ------------- Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2124
On Mon, 18 Jan 2021 12:09:23 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Change `MethodHandles.byteArrayViewVarHandle` to throw `ArrayIndexOutOfBoundsException` rather than the more generic `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to VHs.
CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912)
This pull request has now been integrated. Changeset: 27cc62a5 Author: Claes Redestad <redestad@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/27cc62a5 Stats: 534 lines in 30 files changed: 13 ins; 0 del; 521 mod 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException Reviewed-by: jvernee, mchung ------------- PR: https://git.openjdk.java.net/jdk/pull/2124
participants (4)
-
Claes Redestad
-
Jorn Vernee
-
Mandy Chung
-
Mandy Chung