Re: RFR: 8332003: Clarify javadoc for MemoryLayout::offsetHandle [v2]
On Thu, 9 May 2024 18:26:17 GMT, Maurizio Cimadamore
wrote:
>> Consider this layout:
>>
>>
>> MemoryLayout SEQ = MemoryLayout.sequenceLayout(5,
>> MemoryLayout.sequenceLayout(10, JAVA_INT));
>>
>>
>> And the corresponding offset handle:
>>
>>
>> MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(),
>> PathElement.sequenceLayout());
>>
>>
>> The resulting method handle takes two additional `long` indices. The
>> implementation checks that the dynamically provided indices conform to the
>> bound available at construction: that is, the first index must be < 5, while
>> the second must be < 10. If this is not true, an `IndexOutOfBoundException`
>> is thrown.
>>
>> However, the javadoc for `MemoryLayout::byteOffsetHandle` doesn't claim
>> anything in this direction. There are only some vague claims in the javadoc
>> for `PathElement::sequenceElement()` and `PathElement::sequenceElement(long,
>> long, long)` which make some claims on which indices are actually allowed,
>> but the text seems more in the tone of a discussion, rather than actual
>> normative text.
>>
>> I've tweaked the javadoc for `MemoryLayout::byteOffsetHandle` to actually
>> state that the indices will be checked against the "size" of the
>> corresponding open path element (this is a new concept that I also have
>> defined in the javadoc).
>>
>> I also added a test for `byteOffsetHandle` as I don't think we had a test
>> for that specifically (although this method is tested indirectly, via
>> `MemoryLayout::varHandle`).
>
> Maurizio Cimadamore has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Update copyright
> - Add javadoc to other MemoryLayout methods returning VarHandle/MethodHandle
> to describe which exception can be thrown by returned handle
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 574:
> 572: * {@code c_1}, {@code c_2}, ... {@code c_m} are other
> static offset
> 573: * constants (such as field offsets) which are derived from the
> layout path.
> 574: *
Can we connected back to the prior paragraph? e.g., for any given dynamic
argument `x_n`, the index into the sequence associated with the n'th open path
element whose size is `size_n`,`x_n` must be `>= 0` or `< size_n`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19158#discussion_r1595836040
Re: RFR: 8332003: Clarify javadoc for MemoryLayout::offsetHandle [v2]
On Thu, 9 May 2024 18:26:17 GMT, Maurizio Cimadamore wrote: >> Consider this layout: >> >> >> MemoryLayout SEQ = MemoryLayout.sequenceLayout(5, >> MemoryLayout.sequenceLayout(10, JAVA_INT)); >> >> >> And the corresponding offset handle: >> >> >> MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(), >> PathElement.sequenceLayout()); >> >> >> The resulting method handle takes two additional `long` indices. The >> implementation checks that the dynamically provided indices conform to the >> bound available at construction: that is, the first index must be < 5, while >> the second must be < 10. If this is not true, an `IndexOutOfBoundException` >> is thrown. >> >> However, the javadoc for `MemoryLayout::byteOffsetHandle` doesn't claim >> anything in this direction. There are only some vague claims in the javadoc >> for `PathElement::sequenceElement()` and `PathElement::sequenceElement(long, >> long, long)` which make some claims on which indices are actually allowed, >> but the text seems more in the tone of a discussion, rather than actual >> normative text. >> >> I've tweaked the javadoc for `MemoryLayout::byteOffsetHandle` to actually >> state that the indices will be checked against the "size" of the >> corresponding open path element (this is a new concept that I also have >> defined in the javadoc). >> >> I also added a test for `byteOffsetHandle` as I don't think we had a test >> for that specifically (although this method is tested indirectly, via >> `MemoryLayout::varHandle`). > > Maurizio Cimadamore has updated the pull request incrementally with two > additional commits since the last revision: > > - Update copyright > - Add javadoc to other MemoryLayout methods returning VarHandle/MethodHandle > to describe which exception can be thrown by returned handle In an offline discussion @minborg noted that other methods in the `MemoryLayout` class do not specify in full the set exception thrown by the returned method handle/var handle: * MemoryLayout::varHandle * MemoryLayout::sliceHandle * MemoryLayout::arrayElementVarHandle The first two are missing details re. checks against the upper bound of the index parameter/coordinates. I believe this omission was because these methods already say that if the accessed offset is bigger than the segment size, then a IIOBE occurs. But there's a subtle problem with it. In the impl, even if you have a big enough memory segment to allow access at index 42, but the layout says that the max index is 41, access fails with IIOBE anyway. So I think the javadoc should be updated to contain the new text about the index checks. The last method in the list actually doesn't have any text when it comes to the exceptions thrown by the returned handle. This is because we say that this method can be obtained by calling `collectCoordinates` on `MemoryLayout::varHandle`. Nevertheless, I think it's worth repeating what exceptions can come up (especially as doing so reveals more clearly that there's no extra checks for the very first long coordinate of the returned handle - so now the existing `apiNote` makes even more sense, I think). - PR Comment: https://git.openjdk.org/jdk/pull/19158#issuecomment-2103200882
Re: RFR: 8332003: Clarify javadoc for MemoryLayout::offsetHandle [v2]
> Consider this layout: > > > MemoryLayout SEQ = MemoryLayout.sequenceLayout(5, > MemoryLayout.sequenceLayout(10, JAVA_INT)); > > > And the corresponding offset handle: > > > MethodHandle offsetHandle = SEQ.offsetHandle(PathElement.sequenceLayout(), > PathElement.sequenceLayout()); > > > The resulting method handle takes two additional `long` indices. The > implementation checks that the dynamically provided indices conform to the > bound available at construction: that is, the first index must be < 5, while > the second must be < 10. If this is not true, an `IndexOutOfBoundException` > is thrown. > > However, the javadoc for `MemoryLayout::byteOffsetHandle` doesn't claim > anything in this direction. There are only some vague claims in the javadoc > for `PathElement::sequenceElement()` and `PathElement::sequenceElement(long, > long, long)` which make some claims on which indices are actually allowed, > but the text seems more in the tone of a discussion, rather than actual > normative text. > > I've tweaked the javadoc for `MemoryLayout::byteOffsetHandle` to actually > state that the indices will be checked against the "size" of the > corresponding open path element (this is a new concept that I also have > defined in the javadoc). > > I also added a test for `byteOffsetHandle` as I don't think we had a test for > that specifically (although this method is tested indirectly, via > `MemoryLayout::varHandle`). Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Update copyright - Add javadoc to other MemoryLayout methods returning VarHandle/MethodHandle to describe which exception can be thrown by returned handle - Changes: - all: https://git.openjdk.org/jdk/pull/19158/files - new: https://git.openjdk.org/jdk/pull/19158/files/7cfd222b..aa644d3d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19158&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19158&range=00-01 Stats: 37 lines in 2 files changed: 35 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19158.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19158/head:pull/19158 PR: https://git.openjdk.org/jdk/pull/19158
