On Thu, 1 Jun 2023 18:12:28 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 444:
>
>> 442: * <p>
>> 443: * When an upcall stub is passed to a foreign function, a JVM crash
>> might occur, if the foreign code casts the function pointer
>> 444: * associated with the upcall stub to a type that is incompatible with
>> the type of the upcall stub. Moreover, if the method
>
> This kind of makes it sound like a crash can occur at the time of the cast. I
> think we should mention that the crash can occur when the function is invoked.
> Suggestion:
>
> * When an upcall stub is passed to a foreign function, a JVM crash might
> occur, if the foreign code casts the function pointer
> * associated with the upcall stub to a type that is incompatible with the
> type of the upcall stub, and then attempts to invoke the function through the
> resulting function pointer. Moreover, if the method
I agree that's better - I was also puzzled by the text (I think it's there from
before, just shuffled?)
> src/java.base/share/classes/java/lang/foreign/Linker.java line 473:
>
>> 471:
>> 472: /**
>> 473: * Creates a method handle which is used to call a foreign function
>> with the given signature and symbol.
>
> I always think of a function "symbol" mostly as the _name_ of the function,
> so it seems that "address" would be more correct here. Though, I might be
> wrong on that. It's hard to find a clear definition of "symbol" that applies
> to this specific use case.
the idea behind this is to connect with the javadoc of `SymbolLookup` which
defines and then uses symbol all over the place.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 393:
>
>> 391: * <p>
>> 392: * Multiple paths can be chained, by using <a
>> href=#deref-path-elements>dereference path elements</a>.
>> 393: * A dereference path element allows to obtain a native memory
>> segment whose base address is the address value
>
> "allows to obtain" doesn't sound right to me. I think it should either be
> "allows _subject_ to obtain" (where _subject_ is for instance "the user"), or
> it could also be "allows obtaining" (the the former seems more natural to me).
> Suggestion:
>
> * A dereference path element allows the user to obtain a native memory
> segment whose base address is the address value
I will also look for some other way to say this - typically when I end up with
such convoluted sentences there's a better way to write it :-)
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 418:
>
>> 416: *
>> 417: * @param elements the layout path elements.
>> 418: * @return a var handle that accesses a memory segment at the
>> offset selected by the given layout path.
>
> This doesn't seem quite right. It is not the memory segment which is found at
> the offset given by the layout path, it is the base address.
>
> Maybe:
> Suggestion:
>
> * @return a var handle that accesses a memory segment whose base address
> is found at the offset selected by the given layout path.
Yeah - I meant what you said - but now that you said it, I also saw how what
I've written can be prone to an alternate (and wrong) interpretation. I'll
clarify.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>
>> 468: * @throws IllegalArgumentException if the layout path contains one
>> or more <a href=#deref-path-elements>dereference path elements</a>.
>> 469: * @throws IllegalArgumentException if the layout path contains one
>> or more path elements that select one or more
>> 470: * sequence element indices, such as {@link
>> PathElement#sequenceElement(long)} and {@link
>> PathElement#sequenceElement(long, long)}).
>
> Looks like the first method link should like to
> `PathElement#sequenceElement()` instead? (I think using a bound
> `sequenceElement` is fine right?)
This is deliberate (but subtle). Basically, for `select` we just want to select
a target layout (we don't care which). So the method has a preference for open
sequence layout paths: there's no access or offset to model here, the method
just needs to end up into some layout at the end of the path. One might argue
that these restrictions are unnecessary - but I didn't feel strongly enough to
drop them.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213681006
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679251
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213677679
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213676964
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213675483
