On Mon, 4 Sep 2023 15:54:41 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> This patch contains the implementation of the foreign linker & memory API >> JEP for Java 22. The initial patch is composed of commits brought over >> directly from the [panama-foreign >> repo](https://github.com/openjdk/panama-foreign). The main changes found in >> this patch come from the following PRs: >> >> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous >> iterations supported converting Java strings to and from native strings in >> the UTF-8 encoding, we've extended the supported encoding to all the >> encodings found in the `java.nio.charset.StandardCharsets` class. >> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the >> `MemoryLayout::sequenceLayout` factory method which inferred the size of the >> sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A >> client is now required to explicitly specify the sequence size. >> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added: >> `Linker::canonicalLayouts`, which exposes a map containing the >> platform-specific mappings of common C type names to memory layouts. >> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access >> varhandles, as well as byte offset and slice handles derived from memory >> layouts, now feature an additional 'base offset' coordinate that is added to >> the offset computed by the handle. This allows composing these handles with >> other offset computation strategies that may not be based on the same memory >> layout. This addresses use-cases where clients are working with 'dynamic' >> layouts, whose size might not be known statically, such as variable length >> arrays, or variable size matrices. >> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now >> redundant API. Clients can simply use the difference between the base >> address of two memory segments. >> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of >> `SegmentAllocator::allocateArray`, by renaming methods that both allocate + >> initialize memory segments to `allocateFrom`. (see the original PR for the >> problematic case) >> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the >> documentation for variadic functions. >> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to >> make sure the `jdk_foreign` tests can pass on 32-bit machines, taking >> linux-x86 as a test bed. >> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API >> required. The `Linker::nativeLinker` method is not longer allowed to throw >> an `UnsupportedO... > > Jorn Vernee has updated the pull request incrementally with five additional > commits since the last revision: > > - 8315096: Allowed access modes in memory segment should depend on layout > alignment > > Reviewed-by: psandoz > - Add missing @implSpec to AddressLayout > > Reviewed-by: pminborg > - Fix misc typos in FFM API javadoc > > Reviewed-by: jvernee > - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle > (part two) > > Reviewed-by: pminborg > - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle > > Reviewed-by: jvernee Recommend you do a sweep through the code for unused imports and fields and any rogue `@since` in the internal classes. src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 53: > 51: * > 52: * @implSpec > 53: * This class is immutable, thread-safe and <a > href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>. Strictly speaking it's the implementations, as stated in the `Linker`: * Implementations of this interface are immutable, thread-safe and <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>. src/java.base/share/classes/java/lang/foreign/Linker.java line 152: > 150: * <p> > 151: * The following table shows some examples of how C types are modelled > in Linux/x64 (all the examples provided > 152: * here will assume these platform-dependent mappings): Up to you, but it might be useful to link to the ABI specifications if the links are considered stable. src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 439: > 437: * </ul> > 438: * <p> > 439: * If the provided layout path {@code P} contains no dereference > elements, then the offset of the access operation is Suggestion: * If the provided layout path {@code P} contains no dereference elements, then the offset {@code O} of the access operation is src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 443: > 441: * > 442: * {@snippet lang = "java": > 443: * offset = this.offsetHandle(P).invokeExact(B, I1, I2, ... In); Suggestion: * O = this.offsetHandle(P).invokeExact(B, I1, I2, ... In); To align with the use of `O` later on. src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 536: > 534: * </ul> > 535: * <p> > 536: * The offset of the returned segment is computed as if by a call to > a Suggestion: * The offset {@code O} of the returned segment is computed as if by a call to a src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 154: > 152: * MemoryLayout.sequenceLayout(4, > ValueLayout.JAVA_INT).withName("data") // array of 4 elements > 153: * ); > 154: * VarHandle intHandle = > segmentLayout.varHandle(MemoryLayout.PathElemenet.groupElement("data"), Suggestion: * VarHandle intHandle = segmentLayout.varHandle(MemoryLayout.PathElement.groupElement("data"), src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8027: > 8025: * @since 19 > 8026: */ > 8027: @PreviewFeature(feature=PreviewFeature.Feature.FOREIGN) Unused import to `PreviewFeature`, and possibly others too. src/java.base/share/classes/jdk/internal/foreign/StringSupport.java line 45: > 43: case DOUBLE_BYTE -> readFast_short(segment, offset, charset); > 44: case QUAD_BYTE -> readFast_int(segment, offset, charset); > 45: default -> throw new > UnsupportedOperationException("Unsupported charset: " + charset); Is this necessary, since the switch expression should be exhaustive over all the enum values? ------------- PR Review: https://git.openjdk.org/jdk/pull/15103#pullrequestreview-1611924844 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316401360 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316402470 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316409959 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316410079 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316414805 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316437803 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316457079 PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316444767
