On Tue, 12 Oct 2021 20:51:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix TestLinkToNativeRBP test
Like with previous reviews of code for Panama JEPs there are not many comments
on this PR, since prior reviews occurred for PRs in the panama-foreign repo.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
77:
> 75: * Furthermore, if the function descriptor's return layout is a group
> layout, the resulting downcall method handle accepts
> 76: * an extra parameter of type {@link SegmentAllocator}, which is used by
> the linker runtime to allocate the
> 77: * memory region associated with the struct returned by the downcall
> method handle.
Suggestion:
* memory region associated with the struct returned by the downcall method
handle.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
88:
> 86: * in which the specialized signature of a given variable arity callsite
> is described in full. Alternatively,
> 87: * if the foreign library allows it, clients might also be able to
> interact with variable arity methods
> 88: * using by passing a trailing parameter of type {@link VaList}.
Suggestion:
* by passing a trailing parameter of type {@link VaList}.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
145:
> 143: * Lookup a symbol in the standard libraries associated with this
> linker.
> 144: * The set of symbols available for lookup is unspecified, as it
> depends on the platform and on the operating system.
> 145: * @return a linker-specific library lookup which is suitable to
> find symbols in the standard libraries associated with this linker.
Suggestion:
* @return a symbol in the standard libraries associated with this linker.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java
line 93:
> 91: Objects.requireNonNull(argLayouts);
> 92: Arrays.stream(argLayouts).forEach(Objects::requireNonNull);
> 93: return new FunctionDescriptor(null, argLayouts);
We need to clone `argLayouts`, otherwise the user can modify the contents.
Internally, using `List.of` is useful, since it does the cloning and null
checks, and that is the type that is returned. Also `argumentLayouts` uses
`Arrays.asList` which supports modification of the contents.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java
line 394:
> 392: *
> 393: * The returned allocator might throw an {@link OutOfMemoryError} if
> the total memory allocated with this allocator
> 394: * exceeds the arena size, or the system capacity. Furthermore, the
> returned allocator is not thread safe, and all
The "the returned allocator is not thread safe" contradicts the prior sentence
"An allocator associated with a shared resource scope is thread-safe
and allocation requests may be performed concurrently".
Perhaps just say:
"
The returned allocator is not thread safe if the given scope is a shared scope.
Concurrent allocation needs to be guarded with synchronization primitives.
"
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 260:
> 258:
> 259: @Override
> 260: public final MemorySegment asOverlappingSlice(MemorySegment other) {
Please ignore these comments if you wish.
Adding `max() // exclusive` to complement `min()` might be useful.
In these cases i find it easier to sort the segments e.g. `var a = this; var b
= other; if (a.min() > b.min()) { // swap a and b }` then the subsequent logic
tends to get simpler e.g. overlap test is `if (b.min() < a.max())`,
`segmentOffset` is always +ve.
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ConfinedScope.java
line 100:
> 98: do {
> 99: value = (int)ASYNC_RELEASE_COUNT.getVolatile(this);
> 100: } while (!ASYNC_RELEASE_COUNT.compareAndSet(this, value,
> value + 1));
Use `getAndAdd` (and ignore the return value).
-
PR: https://git.openjdk.java.net/jdk/pull/5907
