On Thu, 1 Jun 2023 15:16:42 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and >> `float` is promoted to `double`, when being passed as variadic argument (see >> e.g. >> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). >> This patch restricts the layouts that can be used as variadic layouts to >> what is allowed by the C specification. >> >> The fallback linker is also updated to use to correct function to link >> variadic calls (not doing this turned out not to be a problem so far, but it >> is problematic for instance on Mac/AArch64 when using the fallback linker). >> Adding the restriction on layouts for all linkers is also partly motivated >> by the fallback linker rejecting such unsupported variadic layouts already. >> >> I've added a small paragraph to the Linker javadoc as well that explains the >> restriction. Comments on that are welcome, but please explain. >> >> The tests are updated to no longer try to link variadic functions with the >> illegal layouts, and I've added some more negative tests to TestIllegalLink. >> >> Testing: >> - local testing on Windows/x64 >> - tier1-3 + jdk-tier5 (ongoing) >> - manual test run on mac/aarch64 with the fallback linker to verify the >> correctness of the fallback linker changes. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Rework javadoc Looks good - I left an optional comment. feel free to ignore (or to deal with that as part of some other PR). src/java.base/share/classes/java/lang/foreign/Linker.java line 373: > 371: * <li>With an empty formal parameter list, such as: {@code void > foo();}</li> > 372: * </ol> > 373: * The latter is often called a <em>prototype-less</em> function as > well. The arguments passed in place of the ellipsis, Would it make sense to move the prototype-less name in the second bullet above? ------------- PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1572258397 PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1213320545
