On Tue, 12 Dec 2023 13:23:27 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR proposes to change the specification for some methods that take
>> `long` offsets so that they will throw an `IllegalArgumentException` rather
>> than an `IndexOutOfBoundsException` for negative values.
>>
>> The PR also proposes to fix a bug where the allocation size would overflow
>> and the specified exception was not thrown.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert change in allocateNoInit
src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 396:
> 394: * @throws IllegalArgumentException if {@code elementCount *
> sourceElementLayout.byteSize()} overflows
> 395: * @throws IndexOutOfBoundsException if {@code sourceOffset >
> source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
> 396: * @throws IndexOutOfBoundsException if either {@code sourceOffset}
> or {@code elementCount} are {@code < 0}
I think that if elementCount < 0, we should throw IAE, not IOOBE. Note that we
explain this method as:
MemorySegment dest = this.allocate(elementLayout, elementCount);
MemorySegment.copy(source, sourceElementLayout, sourceOffset, dest,
elementLayout, 0, elementCount);
So, the set of exceptions thrown by these two methods should be preserved. This
means that `allocate(MemoryLayout, long)` gets a first pass at checking - this
means IAE for overflows and also for negative element count.
After that, the exceptions we can have are those specified in
MemorySegment.copy.
src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 407:
> 405: Objects.requireNonNull(sourceElementLayout);
> 406: Objects.requireNonNull(elementLayout);
> 407: Utils.checkNonNegativeIndex(elementCount, "elementCount");
I think this check should be omitted (see comment above)
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 595:
> 593: long elementCount) {
> 594:
> 595: Utils.checkNonNegativeIndex(srcOffset, "srcOffset");
These new checks don't change the behavior, right? E.g. they will end up
issuing an IIOBE, as before? So, why the changes? (note that these change might
lead to double checks)
src/java.base/share/classes/jdk/internal/foreign/Utils.java line 216:
> 214: }
> 215:
> 216: public static void checkNonNegativeArgument(long value, String name)
> {
Add `@ForceInline` to these
test/jdk/java/foreign/TestSegmentAllocators.java line 230:
> 228: // IllegalStateException if the {@linkplain
> MemorySegment#scope() scope} associated
> 229: // with {@code source} is not {@linkplain
> MemorySegment.Scope#isAlive() alive}
> 230: Arena closedArena = Arena.ofConfined();
the scope/thread assertions are already checked in TestScopedOperations - I
don't think we need this, but you might want to make sure a test case for this
new allocator method is added there
test/jdk/java/foreign/TestSegmentAllocators.java line 248:
> 246: }
> 247:
> 248: // ArithmeticException if {@code elementCount *
> sourceElementLayout.byteSize()} overflows
comment is wrong
test/jdk/java/foreign/TestSegments.java line 194:
> 192:
> 193: @Test(expectedExceptions = IndexOutOfBoundsException.class)
> 194: public void testNegativeGetOffset() {
We have a test specifically for instance segment accessors
(TestMemoryAccessInstance) which covers _all_ accessors - maybe better to move
these new tests there?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424033026
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424035187
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424037078
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424037878
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424041046
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424041984
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1424043834
