RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

[Chris Hegarty]

On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR contains the API and implementation changes for JEP-412 [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/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address first batch of review comments

Like Paul, I tracked the changes to this API in the Panama repo. Most of my remaining comments are small and come from re-reading the API docs.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 270:

> 268: 
> 269:     /**
> 270:      * Converts a Java string into a null-terminated C string, using the platform's default charset,

Sorry if this has come up before, but, is the platform's default charset the right choice here? For other areas, we choose UTF-8 as the default. In fact, there is a draft JEP to move the default charset to UTF-8. So if there is an implicit need to match the underlying platform's charset then this may need to be revisited.  For now, I just want to check that this is not an accidental reliance on the platform's default charset, but a deliberate one.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 101:

> 99:  * <h2>Lifecycle and confinement</h2>
> 100:  *
> 101:  * Memory segments are associated to a resource scope (see {@link ResourceScope}), which can be accessed using

Typo?? "Memory segments are associated *with* a resource scope"

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 112:

> 110: MemoryAccess.getLong(segment); // already closed!
> 111:  * }</pre></blockquote>
> 112:  * Additionally, access to a memory segment in subject to the thread-confinement checks enforced by the owning scope; that is,

Typo?? "Additionally, access to a memory segment *is* subject to ..."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 114:

> 112:  * Additionally, access to a memory segment in subject to the thread-confinement checks enforced by the owning scope; that is,
> 113:  * if the segment is associated with a shared scope, it can be accessed by multiple threads; if it is associated with a confined
> 114:  * scope, it can only be accessed by the thread which own the scope.

Typo?? "it can only be accessed by the thread which ownS the scope."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 693:

> 691:      */
> 692:     static MemorySegment allocateNative(MemoryLayout layout, ResourceScope scope) {
> 693:         Objects.requireNonNull(scope);

Should the allocateNative methods declare that they throw ISE, if the given ResourceScope is not alive?   ( I found myself asking this q, then considering the behaviour of a SegmentAllocator that is asked to allocate after a RS has been closed )

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

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3699