On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore <[email protected]>
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
