RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]

Thu Jun 1 13:42:18 UTC 2023

On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> As the FFM API evolved over time, some parts of the javadoc went out of sync. Now that most of the API is stable, it is a good time to look again at the javadoc as a whole, and bring some more consistency.
>> 
>> While most of the changes in this PR are stylistic, I'd like to call out few things which resulted in API changes:
>> 
>> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified requirement that its alignment parameter must be a power of two.
>> 
>> * `MemoryLayout::sliceHandle` did not require the absence of dereference paths in the provided layout path. While that is technically possible to allow, currently the method is specified as returning a "slice" corresponding to some "nested layout", so following pointers seems incompatible with the spec. I have decided to disallow for now - we can always compatibly enable it later, if required.
>> 
>> * `MemorySegment::copy` - most of the overloads did not specify that `UnsupportedOperationException` is thrown if the destination segment is read-only.
>> 
>> * In several places, an extra `@throws IllegalArgumentException` or `@throws IllegalArgumentException` has been added to capture cases where element size * index computation can overflow. This is true for all the element-wise segment copy methods, for the indexed accessors in memory segment (e.g. `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for `SegmentAllocator::allocateArray(MemoryLayout, long)`.
>> 
>> In all these cases (except for overflows), new tests have been added to cover the additional API changes (a CSR will also be filed to cover these).
>> 
>> The class with most changes is `MemoryLayout`. I realized that the javadoc there was a bit sloppy around the definition of "layout paths". Now there are several subsection in the class javadoc, which explain how different kinds of paths can be used. I have introduced the notion of "open path elements" to denote those path elements that are not fully specified, and result in additional var handle coordinates to be added. There is also now a definition of what it means for a layout path to be "well-formed", so that all methods accepting a layout path can simply refer to it (this definition is tricky to give inline in the javadoc of the various path-accepting methods, as it depends on many factors).
>> 
>> Another biggie change was in `MemorySegment` as now all dereference methods state precisely their spatial checks requirements, as well also specifying when the API can th...
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix wrong link in layout well-formedness doc

src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 116:

> 114:     /**
> 115:      * Returns an address layout with the same carrier, alignment constraint, name and order as this address layout,
> 116:      * but with no target layout

Did you mean to drop the period from the end of the sentence?

src/java.base/share/classes/java/lang/foreign/Arena.java line 269:

> 267:      * @throws IllegalStateException if this arena has already been {@linkplain #close() closed}.
> 268:      * @throws WrongThreadException if this arena is confined, and this method is called from a thread {@code T}
> 269:      * other than the arena's owner thread.

"thread T" hints that "T" will be used later, maybe it's not needed.

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 38:

> 36: /**
> 37:  * A function descriptor models the signature of a foreign function. A function descriptor is made up of zero or more
> 38:  * argument layouts and zero or one return layout. A function descriptor is used to create

You might want want to put a comma after "layouts" to avoid any ambiguity.

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 57:

> 55: 
> 56:     /**
> 57:      * {@return the argument layouts of this function descriptor (as an immutable list)}.

I assume this should be "unmodifiable" rather than immutable here.

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 127:

> 125:     /**
> 126:      * Creates a function descriptor with the given argument layouts and no return layout.  This is useful to model functions
> 127:      * which return no values.

I think I would use "that return" rather than "which return" here.

src/java.base/share/classes/java/lang/foreign/Linker.java line 356:

> 354:  * attach the segment to some existing {@linkplain Arena arena}, so that the lifetime of the region of memory
> 355:  * backing the segment can be managed automatically, as for any other native segment created directly from Java code.
> 356:  * Both these operations are accomplished using the restricted method {@link MemorySegment#reinterpret(long, Arena, Consumer)},

I think this should be "Both of these operations".

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 325:

> 323:      * @return a segment for the newly allocated memory block.
> 324:      * @throws IllegalArgumentException if {@code elementLayout.byteSize() * count} overflows.
> 325:      * @throws IllegalArgumentException if {@code count < 0}.

Another case where the IAE descriptions should probably be combined.

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 363:

> 361:      * The returned allocator throws {@link IndexOutOfBoundsException} when a slice of the provided
> 362:      * segment with the requested size and alignment cannot be found.
> 363:      * @implNote A slicing allocator is not <em>thread-safe</em>.

The implNote about thread safety makes me wonder if the SegmentAllocator should say something about thread safety, e.g. should it specify that the allocate methods are thread safe?

src/java.base/share/classes/java/lang/foreign/SequenceLayout.java line 75:

> 73:      * @return a sequence layout with the same characteristics of this layout, but with the given element count.
> 74:      * @throws IllegalArgumentException if {@code elementCount} is negative.
> 75:      * @throws IllegalArgumentException if {@code elementLayout.bitSize() * elementCount} overflows.

Shouldn't the exception descriptions be combined to avoid IAE being listed twice in the generated javadoc?

src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 57:

> 55:  *     <li>It can be {@linkplain MemorySegment#set(AddressLayout, long, MemorySegment) stored} inside another memory segment.</li>
> 56:  *     <li>It can be used to access the region of memory backing a global variable (this requires
> 57:  *     {@link MemorySegment#reinterpret(long) resizing} the segment first).</li>

This one probably should be linkplain.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213115981
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213119694
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213122457
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213123988
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213126349
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213130078
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213141790
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213171901
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213140568
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213138835