On Wed, 22 Mar 2023 14:09:07 GMT, Per Minborg <pminb...@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
