8346657: Improve out of bounds exception messages for MemorySegments #28124
Conversation
|
👋 Welcome back hextriclosan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
@hextriclosan The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| private IndexOutOfBoundsException outOfBoundException(long offset, long length) { | ||
| return new IndexOutOfBoundsException(String.format("Out of bound access on segment %s; new offset = %d; new length = %d", | ||
| this, offset, length)); | ||
| return new IndexOutOfBoundsException(String.format("Out of bound access on segment %s; " + |
There was a problem hiding this comment.
The new message looks ok, but note that this message is also reused by the asSlice methods. If we want to clarify the messages, perhaps we should think about rearranging the code a bit?
Related note: this is also why the routine that check OOB access needs to worry about length being zero -- because that's a valid value for asSlice (but not for access).
| throw outOfBoundException(offset, length); | ||
| } | ||
| void checkSliceBounds(long offset, long length) { | ||
| Preconditions.checkFromIndexSize(offset, length, this.length, this::sliceOutOfBoundException); |
There was a problem hiding this comment.
Is this performance-wise neutral? This is on a hot path and captures a lambda every time, need to verify compiler can eliminate this, which I doubt.
… should be eliminated by escape analysis
3 participants
Logic for creating IndexOutOfBoundsException in MemorySegment is reworked:
accessandslicecasesProgress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28124/head:pull/28124$ git checkout pull/28124Update a local copy of the PR:
$ git checkout pull/28124$ git pull https://git.openjdk.org/jdk.git pull/28124/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28124View PR using the GUI difftool:
$ git pr show -t 28124Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28124.diff
Using Webrev
Link to Webrev Comment