On Tue, 8 Nov 2022 13:28:58 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: > > Rework package-level javadoc for restricted methods Did a full review. Only some minor comments. Also, please add attribution with `/contributor add @<user>` for the people that contributed. (I think you have to add yourself as well, if you do that). src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 57: > 55: > 56: @Override > 57: GroupLayout withName(String name); It looks like this method, and `withBitAlignment` below have no javadoc? Does this need `inheritDoc`? src/java.base/share/classes/java/lang/foreign/Linker.java line 75: > 73: * <ul> > 74: * <li>if {@code L} is a {@link ValueLayout} with carrier {@code E} then > {@code C = E}; or</li> > 75: * <li>if {@code L} is a {@link GroupLayout}, then {@code C} is set to > {@code MemorySegment.class}</li> Now that we have `FunctionDescriptor::toMethodType` I think this paragraph could be simplified by just referencing that. src/java.base/share/classes/java/lang/foreign/Linker.java line 101: > 99: * <ul> > 100: * <li>if {@code L} is a {@link ValueLayout} with carrier {@code E} then > {@code C = E}; or</li> > 101: * <li>if {@code L} is a {@link GroupLayout}, then {@code C} is set to > {@code MemorySegment.class}</li> Same here. This is covered by the doc of `FunctionDescriptor::toMethodType`. src/java.base/share/classes/java/lang/foreign/Linker.java line 119: > 117: * <li>The memory session of {@code A} is {@linkplain > MemorySession#isAlive() alive}. Otherwise, the invocation throws > 118: * {@link IllegalStateException};</li> > 119: * <li>The invocation occurs in same thread as the one {@linkplain > MemorySession#isOwnedBy(Thread) owning} the memory session of {@code R}, Suggestion: * <li>The invocation occurs in same thread as the one {@linkplain MemorySession#isOwnedBy(Thread) owning} the memory session of {@code A}, src/java.base/share/classes/java/lang/foreign/Linker.java line 121: > 119: * <li>The invocation occurs in same thread as the one {@linkplain > MemorySession#isOwnedBy(Thread) owning} the memory session of {@code R}, > 120: * if said session is confined. Otherwise, the invocation throws > {@link WrongThreadException}; and</li> > 121: * <li>The memory session of {@code R} is <em>kept alive</em> (and > cannot be closed) during the invocation.</li> Suggestion: * <li>The memory session of {@code A} is <em>kept alive</em> (and cannot be closed) during the invocation.</li> src/java.base/share/classes/java/lang/foreign/StructLayout.java line 43: > 41: > 42: @Override > 43: StructLayout withName(String name); Missing `inheritDoc`? src/java.base/share/classes/java/lang/foreign/UnionLayout.java line 43: > 41: > 42: @Override > 43: UnionLayout withName(String name); Missing `inheritDoc`? src/java.base/share/classes/java/lang/foreign/VaList.java line 44: > 42: * Helper class to create and manipulate variable argument lists, similar > in functionality to a C {@code va_list}. > 43: * <p> > 44: * A variable argument list segment can be created using the {@link > #make(Consumer, MemorySession)} factory, as follows: Suggestion: * A variable argument list can be created using the {@link #make(Consumer, MemorySession)} factory, as follows: src/java.base/share/classes/java/lang/foreign/VaList.java line 50: > 48: * .addVarg(C_DOUBLE, 3.8d)); > 49: *} > 50: * Once created, clients can obtain the platform-dependent {@linkplain > #segment() memory segment} associated a variable Suggestion: * Once created, clients can obtain the platform-dependent {@linkplain #segment() memory segment} associated with a variable src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 134: > 132: > 133: @Override > 134: ValueLayout withName(String name); Missing `inheritDoc` here as well, and on other withers below. src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 356: > 354: * Equivalent to the following code: > 355: * {@snippet lang=java : > 356: * ADDRESS.of(ByteOrder.nativeOrder()) This code doesn't look correct. It also looks like OfAddress layouts have their alignment set to the address size already, so the alignment adjustment here seems unnecessary as well. src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 367: > 365: * Equivalent to the following code: > 366: * {@snippet lang=java : > 367: * JAVA_BYTE.of(ByteOrder.nativeOrder()).withBitAlignment(8); Same here (and for the other snippets below), `OfByte` doesn't have an `of` method. This looks maybe like a regex-replace error. ------------- PR: https://git.openjdk.org/jdk/pull/10872
