On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/419 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak javadoc of loaderLookup Mostly some minor javadoc comments. src/java.base/share/classes/java/lang/Module.java line 32: > 30: import java.lang.annotation.Annotation; > 31: import java.lang.invoke.MethodHandle; > 32: import java.lang.invoke.VarHandle; These imports seem spurious now. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 177: > 175: } > 176: if (carrier.isPrimitive() && > Wrapper.forPrimitiveType(carrier).bitWidth() != size && > 177: carrier != boolean.class && size != 8) { I find this condition hard to parse, I'd suggest re-writing it as: if (carrier.isPrimitive()) { long expectedSize = carrier == boolean.class ? 8 : Wrapper.forPrimitiveType(carrier).bitWidth(); if (size != expectedSize) { throw ... } } (Maybe even change the `if` to an `else` and combine it with the above if). src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 484: > 482: public static final class OfAddress extends ValueLayout { > 483: OfAddress(ByteOrder order) { > 484: super(MemoryAddress.class, order, Unsafe.ADDRESS_SIZE * 8); I see `Unsafe.ADDRESS_SIZE` used in several places, suggest to maybe add an `ADDRESS_SIZE_BITS` constants somewhere (it's a bit more readable). src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 42: > 40: final long blockSize; > 41: final long arenaSize; > 42: final ResourceScope scope; Could these field be made private? src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 88: > 86: if (size > arenaSize) { > 87: throw new OutOfMemoryError(); > 88: } Isn't this already covered by the `finally` block? Also, this seems to be checking the unaltered `size`, which I think should have been already done at the end of the previous `allocate` call right? src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java line 122: > 120: ResourceScopeImpl targetImpl = (ResourceScopeImpl)target; > 121: targetImpl.acquire0(); > 122: addCloseAction(targetImpl::release0); Maybe this should explicitly check if target is `null` (though the call to `acquire0` would also produce an NPE, the stack trace having Objects::requireNonNull in there would make the error more obvious I think). Suggestion: public void keepAlive(ResourceScope target) { Objects.requireNonNull(target); if (target == this) { throw new IllegalArgumentException("Invalid target scope."); } ResourceScopeImpl targetImpl = (ResourceScopeImpl)target; targetImpl.acquire0(); addCloseAction(targetImpl::release0); src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java line 101: > 99: int value; > 100: do { > 101: value = (int) > STATE.getVolatile(jdk.internal.foreign.SharedScope.this); Doesn't need to be fully qualified I think? Suggestion: value = (int) STATE.getVolatile(this); src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java line 106: > 104: throw new IllegalStateException("Already closed"); > 105: } > 106: } while > (!STATE.compareAndSet(jdk.internal.foreign.SharedScope.this, value, value - > 1)); Same here Suggestion: } while (!STATE.compareAndSet(this, value, value - 1)); ------------- Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5907
