RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]

[Maurizio Cimadamore]

On Thu, 1 Jun 2023 18:12:28 GMT, Jorn Vernee <jvernee at 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