RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]

[Jorn Vernee]

On Wed, 22 Mar 2023 14:09:07 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> API changes for the FFM API (third preview)
>> 
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>> 
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve javadocs for Linker::captureStateLayout

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

> 490:      * Finally, the returned method handle will throw an {@link IllegalArgumentException} if the {@link MemorySegment}
> 491:      * parameter passed to it is associated with the {@link MemorySegment#NULL} address, or a {@link NullPointerException}
> 492:      * if that parameter is {@code null}.

I think this isn't quite correct, as it only applies to the target address parameter. Also, we don't have to mention the NPE, as that's already mentioned in the package doc
Suggestion:

     * Finally, the returned method handle will throw an {@link IllegalArgumentException} if the {@link MemorySegment}
     * representing the target address of the foreign function is the {@link MemorySegment#NULL} address.

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

> 2313:          * @return {@code true}, if the provided object is also a scope, which models the same lifetime as that
> 2314:          * modelled by this scope.
> 2315:          */

A `@return` tag could be used here
Suggestion:

        /**
         * {@return {@code true}, if the provided object is also a scope, which models the same lifetime as that
         * modelled by this scope.} In that case, it is always the case that
         * {@code this.isAlive() == ((Scope)that).isAlive()}.
         * @param that the object to be tested.
         */

src/java.base/share/classes/jdk/internal/foreign/abi/fallback/FFIType.java line 116:

> 114:             }
> 115:             assert grpl instanceof UnionLayout;
> 116:             throw new IllegalArgumentException("Fallback linker does not support by-value unions: " + grpl);

For posterity, I suggest adding a comment here with the the bug number.
Suggestion:

            // JDK-8301800
            throw new IllegalArgumentException("Fallback linker does not support by-value unions: " + grpl);

src/java.base/share/native/libfallbackLinker/fallbackLinker.c line 82:

> 80:     value_ptr++;
> 81:   }
> 82: #endif

This looks incorrect. Look at the changes to `downcallLinker.cpp`, the `value_ptr++` should be moved outside of the if statements. (it only matters on Windows though, which we can't test at the moment for the fallback linker, but let's keep the 2 impls in sync for now)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145188849
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145221286
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145245662
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145258121