Re: RFR: 8310405: Linker.Option.firstVariadicArg should specify which index values are valid [v2]
> Improve the specification of `Linker.Option.firstVariadicArg`, by specifying > more clearly which index values are valid. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: polish doc, review comments - Changes: - all: https://git.openjdk.org/jdk/pull/14565/files - new: https://git.openjdk.org/jdk/pull/14565/files/24bf486f..f6a111b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14565&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14565&range=00-01 Stats: 17 lines in 1 file changed: 5 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/14565.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14565/head:pull/14565 PR: https://git.openjdk.org/jdk/pull/14565
Re: RFR: 8310405: Linker.Option.firstVariadicArg should specify which index values are valid [v2]
On Wed, 21 Jun 2023 07:34:20 GMT, Per Minborg wrote: >> Possible suggestion/thing to try: use a bullet list to spell out all cases >> for `index`. E.g. we know there's index == 0 (all variadic). Then we know >> there's index = N (no variadic). Then there's index == m, 0 < m < N - which >> means layouts 0..m are non-variadic and m..N are variadic (where n..m >> denotes an interval with n included and m excluded). > >> Possible suggestion/thing to try: use a bullet list to spell out all cases >> for `index`. E.g. we know there's index == 0 (all variadic). Then we know >> there's index = N (no variadic). Then there's index == m, 0 < m < N - which >> means layouts 0..m are non-variadic and m..N are variadic (where n..m >> denotes an interval with n included and m excluded). > > I think this is a good suggestion. It makes it much easier to understand. Thanks for the review. I've added a bullet list, and switch same of the language to refer to the 'start of the variadic arguments passed to the function described by the function descriptor'. I think the latter avoids implying the index is an index into the argument layouts, but it feels like a bit of a mouthful (any suggestions?). I've also added a small note to the global variadic function doc to indicate that the index might not necessarily have a corresponding argument layout. How does the new version look? - PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1237819366
Re: RFR: 8310405: Linker.Option.firstVariadicArg should specify which index values are valid [v2]
On Wed, 21 Jun 2023 23:33:37 GMT, Jorn Vernee wrote:
>> Improve the specification of `Linker.Option.firstVariadicArg`, by specifying
>> more clearly which index values are valid.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> polish doc, review comments
src/java.base/share/classes/java/lang/foreign/Linker.java line 654:
> 652: *
> 653: * {@code 0}, all arguments passed to the function are
> passed as variadic arguments
> 654: * {@code N}, none of the arguments passed to the function
> are passed as variadic argument
Suggestion:
* {@code N}, none of the arguments passed to the function are
passed as variadic argumenst
src/java.base/share/classes/java/lang/foreign/Linker.java line 658:
> 656: *
> 657: * It is important to always use this linker option when linking
> a variadic
> 658: * function, even when none of the arguments are passed as
> variadic arguments (the second case in the list
Suggestion:
* function, even if no variadic argument is passed (the second
case in the list
src/java.base/share/classes/java/lang/foreign/Linker.java line 659:
> 657: * It is important to always use this linker option when linking
> a variadic
> 658: * function, even when none of the arguments are passed as
> variadic arguments (the second case in the list
> 659: * above), as this affects the linking process on certain
> platforms.
Suggestion:
* above), as this might still affect the calling convention on certain
platforms.
src/java.base/share/classes/java/lang/foreign/Linker.java line 664:
> 662: * against which the index is validated is available.
> 663: *
> 664: * @param index the index indicating the start of the variadic
> arguments passed to the function described by
My general feeling is that the previous text was better here. We describe in
plenty details what this "index" is in the method javadoc. I think I'm more for
reverting the changes here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1240079184
PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1240081468
PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1240083169
PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1240087788
Re: RFR: 8310405: Linker.Option.firstVariadicArg should specify which index values are valid [v2]
On Fri, 23 Jun 2023 17:24:30 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> polish doc, review comments > > src/java.base/share/classes/java/lang/foreign/Linker.java line 659: > >> 657: * It is important to always use this linker option when >> linking a variadic >> 658: * function, even when none of the arguments are passed as >> variadic arguments (the second case in the list >> 659: * above), as this affects the linking process on certain >> platforms. > > Suggestion: > > * above), as this might still affect the calling convention on > certain platforms. Note: we use the term calling convention in Linker javadoc - I think it's slightly more precise than "linking process". - PR Review Comment: https://git.openjdk.org/jdk/pull/14565#discussion_r1240083929
