On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make memory session a pure lifetime abstraction src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 157: > 155: public long mismatch(MemorySegment other) { > 156: Objects.requireNonNull(other); > 157: return MemorySegment.mismatch(this, 0, byteSize(), other, 0, > other.byteSize()); Bit strange to see this calling back up to a method in the interface. Maybe this should just be a `default` method in `MemorySegment`? src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 163: > 161: * Mismatch over long lengths. > 162: */ > 163: public static long vectorizedMismatchLargeForBytes(MemorySessionImpl > aSession, MemorySessionImpl bSession, Does this need to be `public`? Only seems to be referenced below. src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 179: > 177: @ForceInline > 178: public static MemorySessionImpl toSessionImpl(MemorySession session) > { > 179: return (MemorySessionImpl)session; Maybe calls to this method should just be replaced with a cast. src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 136: > 134: long ptr = UNSAFE.allocateMemory(LAYOUT.byteSize()); > 135: MemorySegment ms = MemorySegment.ofAddress(ptr, > LAYOUT.byteSize(), > 136: MemorySession.implicit(), () -> UNSAFE.freeMemory(ptr)); pre-existing, but it seems like this could just use `MemorySegment.allocateNative(LAYOUT, MemorySession.implicit())`? Suggestion: MemorySegment base = MemorySegment.allocateNative(LAYOUT, MemorySession.implicit()); (and remove the dependency on `Unsafe` altogether) src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 142: > 140: VH_gr_offs.set(ms, 0); > 141: VH_vr_offs.set(ms, 0); > 142: return ms; I suggest doing Suggestion: return ms.asSlice(0, 0); To create an opaque segment, just like the `segment()` accessor does. Or maybe update the implementation of `SharedUtils.emptyVaList` to do this. src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 408: > 406: @Override > 407: public MemorySegment segment() { > 408: return segment.asSlice(0, 0); A comment about what is happening here would be nice. (making sure the returned segment is opaque?) src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64VaList.java line 176: > 174: @Override > 175: public MemorySegment segment() { > 176: return segment.asSlice(0, 0); Same here. src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 145: > 143: long ptr = U.allocateMemory(LAYOUT.byteSize()); > 144: MemorySegment base = MemorySegment.ofAddress(ptr, > LAYOUT.byteSize(), > 145: MemorySession.implicit(), () -> U.freeMemory(ptr)); Same here: `MemorySegment base = MemorySegment.allocateNative(LAYOUT, MemorySession.implicit());` src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 150: > 148: VH_overflow_arg_area.set(base, MemorySegment.NULL); > 149: VH_reg_save_area.set(base, MemorySegment.NULL); > 150: return base; Suggestion: return base.asSlice(0, 0); test/jdk/java/foreign/normalize/TestNormalize.java line 203: > 201: public static Object[][] bools() { > 202: return new Object[][]{ > 203: { 0b01, true }, // zero least significant bit, but > non-zero first byte According to the comment this should actually be: Suggestion: { 0b10, true }, // zero least significant bit, but non-zero first byte Looks like I wrote this by mistake :( ------------- PR: https://git.openjdk.org/jdk/pull/10872
