On Mon, 23 Sep 2024 10:51:10 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
>> line 225:
>>
>>> 223: // check elements are not all padding layouts and for trailing
>>> padding
>>> 224: private void checkGroup(GroupLayout gl, long maxUnpaddedOffset) {
>>> 225: if (gl.memberLayouts().stream().allMatch(e -> e instanceof
>>> PaddingLayout)) {
>>
>> I'm not 100% sure that this check (which I agree with) can be desumed from
>> the current `Linker` javadoc. E.g. if I have:
>>
>> `groupLayout(paddingLayout(1))`
>>
>> The Linker says:
>> * there cannot be any intermediate padding more than necessary to align
>> non-padding elements. But there's no non-padding elements here, so <shrug>
>> :-)
>> * there cannot be more padding at the end, more than necessary to make the
>> group size a multiple of its alignment - but again, since there's no
>> non-padding layout, it is not clear what this means here. We have a padding
>> of 1, which makes the group have size 1, which is a multiple of the
>> alignment of the struct. So, ok?
>
> Previously, the check threw an `IllegalArgumentException` when presented with
> the above (or other) group layout with only padding layouts, but the actual
> message differed. So, this change only changes the exception message and does
> not change the current behavior. However, the question remains: Should we
> update the docs to reflect this behavior?
Note: I know the behavior in this particular case is not affected by the PR.
That said, since you already need CSR for the "sequence with padding"
behavioral change, I think it could make sense to piggy back on this PR, and
clarify the javadoc for the other case as well.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1771216722
