[foreign-preview] RFR: 8280527: Move jdk.incubator.foreign to java.lang.foreign

[Maurizio Cimadamore]

On Mon, 24 Jan 2022 16:34:34 GMT, Julia Boes <jboes at openjdk.org> wrote:

> To prepare for preview, this change removes the jdk.incubator.foreign module and moves the public classes to java.base/java.lang.foreign, implementation classes to java.base/jdk.internal.foreign respectively. 
> 
> While here, `SystemLookup` is replaced with the method `ClassLoader::findNative` and the methods in `MemoryHandles` are moved to `MethodHandles`. The `@PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)` annotation and the `--enable-preview` flag are added were needed.

Looks good. I've added some comments. As for the package name, perhaps using a less nested name `java.foreign` would make it easier to update it later on (if needs be). The `lang` in `java.lang.foreign` is a bit misleading, as @JornVernee pointed out (as there's nothing directly related to the language here). But if it's too hard to fix we can look at tweaking that later.

src/hotspot/share/prims/scopedMemoryAccess.cpp line 173:

> 171: 
> 172: #undef CC
> 173: #undef FN_PTR

Since we're here (but it's ok if we defer it to another PR) we could get rid of the trailing exception parameter in `ScopedMemoryAccess_closeScope`. This was used in a previous iteration of the implementation, but now it's just an unused parameter.

src/java.base/share/classes/java/lang/ClassLoader.java line 2471:

> 2469:     // A resource scope which keeps this loader reachable. Useful when returning
> 2470:     // native symbols associated with libraries loaded by this loader.
> 2471:     final ResourceScope loaderScope = ResourceScopeImpl.heapScope(this);

should be `private` ?

src/java.base/share/classes/java/lang/foreign/CLinker.java line 111:

> (failed to retrieve contents of file, check the PR for context)
I would retain this section:


<h2>Symbol lookup</h2>
Clients can {@linkplain #lookup(String) look up} symbols in the standard libraries associated with this linker. The set of symbols available for lookup is unspecified, as it depends on the platform and on the operating system.

src/java.base/share/classes/java/lang/foreign/CLinker.java line 166:

> 164:      * Look up a symbol in the standard libraries associated with this linker.
> 165:      * The set of symbols available for lookup is unspecified, as it depends on the platform and on the operating system.
> 166:      * @param name the name of the symbol

Suggestion:

     * @param name the symbol name.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 138:

> 136:  * by providing a so called <a href="MemoryLayout.html#layout-paths"><em>layout path</em></a>.
> 137:  * Alternatively, clients can obtain raw memory access var handles from a given
> 138:  * {@linkplain java.lang.invoke.MethodHandles#memoryAccessVarHandle(ValueLayout) value layout}, and then adapt it using the var handle combinator

It seems like the general trend of this patch is to make `@link` shorter by using import statements (which I appreciate a lot!) - should we do the same here?

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7929:

> 7927:      */
> 7928:     @PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)
> 7929:     public static VarHandle memoryAccessVarHandle(ValueLayout layout) {

I was looking at the byte buffer VH factory which is called `byteBufferViewVarHandle` - perhaps we should use a similar scheme like `segmentViewVarHandle` ? (we can address this separately)

src/java.base/share/classes/java/lang/invoke/VarHandles.java line 56:

> 54:     static {
> 55:         try {
> 56:             BYTE_TO_BOOL = MethodHandles.lookup().findStatic(VarHandles.class, "byteToBoolean",

Where are these used? I think `Utils` declares its own set of MHs ?

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 366:

> 364: 
> 365:     void checkValidState() {
> 366:         try {

Why the exception wrapping?

test/jdk/java/foreign/TestDowncall.java line 87:

> 85:     public void testDowncallStack(int count, String fName, Ret ret, List<ParamType> paramTypes, List<StructFieldType> fields) throws Throwable {
> 86:         List<Consumer<Object>> checks = new ArrayList<>();
> 87:         NativeSymbol addr = TestDowncall.class.getClassLoader().findNative("s" +    fName).get();

Extra space

test/micro/org/openjdk/bench/java/lang/foreign/BulkOps.java line 179:

> 177:     @OutputTimeUnit(TimeUnit.NANOSECONDS)
> 178:     public void segment_copy_static_dontinline() {
> 179:         MemorySegment.copy(bytes, 0, segment, JAVA_BYTE, 0, bytes.length);

Note that `bytes` has type `int[]`, so using `JAVA_INT` was the right choice. Maybe you just want to rename the `bytes` field to `int` ?

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

PR: https://git.openjdk.java.net/panama-foreign/pull/633