On Fri, 4 Nov 2022 18:23:17 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 with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 17 additional > commits since the last revision: > > - Merge branch 'master' into PR_20 > - Merge branch 'master' into PR_20 > - Merge pull request #14 from minborg/small-javadoc > > Update some javadocs > - Update some javadocs > - Revert some javadoc changes > - Merge branch 'master' into PR_20 > - Fix benchmark and test failure > - Merge pull request #13 from minborg/revert-factories > > Revert MemorySegment factories > - Update javadocs after comments > - Revert MemorySegment factories > - ... and 7 more: https://git.openjdk.org/jdk/compare/e1e4e45b...3d933028 Some preliminary comments about some changes I think are missing from this PR (noticed while I was making a patch for the VM changes) I will do a more thorough review after the changes from https://github.com/openjdk/panama-foreign/pull/750 are included as well. src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 474: > 472: long bbAddress = NIO_ACCESS.getBufferAddress(bb); > 473: Object base = NIO_ACCESS.getBufferBase(bb); > 474: UnmapperProxy unmapper = NIO_ACCESS.unmapper(bb); Looks like here is also missing the fix that rejects StringCharBuffer: https://github.com/openjdk/panama-foreign/pull/741 I think that is good to include as well. src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 477: > 475: case UNBOX_ADDRESS -> emitUnboxAddress(); > 476: case DUP -> emitDupBinding(); > 477: case CAST -> emitCast((Binding.Cast) binding); This contains the CAST binding, but not the accompanying VM changes from: https://github.com/openjdk/panama-foreign/pull/720 which removes the now dead code. Preferably both changes go together (and the code removal is pretty trivial, so I suggest including it here) src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 491: > 489: emitLoad(highLevelType, paramIndex2ParamSlot[paramIndex]); > 490: > 491: if (shouldAcquire(paramIndex)) { I can't comment on the actual line below, but this is also missing the fix from: https://github.com/openjdk/panama-foreign/pull/739 (that is a Java-only change as well). I suggest adding that as well. src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java line 165: > 163: assert forArguments : "no stack returns"; > 164: // stack > 165: long alignment = Math.max(layout.byteAlignment(), > STACK_SLOT_SIZE); This is also missing part of the changes from: https://github.com/openjdk/panama-foreign/pull/728/ but other changes to the shared code are present. The `layout` parameter is not needed here. (see the changes to this file in the original PR) ------------- PR: https://git.openjdk.org/jdk/pull/10872
