On Mon, 23 Sep 2024 10:19:43 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Per Minborg has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains nine additional
>> commits since the last revision:
>>
>> - Add specific message for group layouts with only padding layouts
>> - Merge branch 'master' into linker-padding-layout-only
>> - Check exception message
>> - Remove redundant doc section
>> - Merge branch 'master' into linker-padding-layout-only
>> - Improve excception message
>> - Document a sequence layout cannot contain a padding layout
>> - Update copyright year
>> - Prevent embeded only padding layout in linker
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1771187107
