[foreign-memaccess+abi] RFR: 8291826: Rework MemoryLayout Sealed Hierarchy [v8]

[Paul Sandoz]

On Tue, 23 Aug 2022 07:29:20 GMT, Per Minborg <duke at openjdk.org> wrote:

>> This PR is a significant refactor of the `MemoryLayout` sealed hierarchy and whose purpose is described more in detail in  https://bugs.openjdk.org/browse/JDK-8291826. In short, the main purpose is to allow pattern matching.
>> 
>> The objective of the PR is to fix the issues described above while retaining as much API compatibility as possible with the pre-PR API. No changes were needed in any of the existing tests for example.
>> 
>> The old sealed hierarchy prevented pattern matching with totality from being used and implied a number of other problems as illustrated in the picture below:
>> 
>> ![graphviz (33)](https://user-images.githubusercontent.com/7457876/185412743-febeb3cf-aebc-4e2e-b78f-3b8ef87b0510.png)
>> 
>> Red nodes cannot be used in pattern matching. Light red nodes are implementations that are visible in the API. White nodes are interfaces.
>> 
>> After this PR has been integrated, an improved sealed hierarchy will exist as depicted here:
>> 
>> ![graphviz (34)](https://user-images.githubusercontent.com/7457876/185413448-8435b23f-5ac4-4999-8b0a-0d6051a0d9be.png)
>> 
>> Gray "Impl" nodes are internal and not exported by the `java.base` module. White nodes are interfaces.
>> 
>> Below, the "Principal Totalities" for the new API are outlined:
>> 
>> 
>> var v0 = switch (memoryLayout) {
>>   case MemoryLayout ml -> 0;
>> };
>> var v1 = switch (memoryLayout) {
>>   case GroupLayout gl -> 0;
>>   case PaddingLayout pl -> 0; // leaf
>>   case SequenceLayout sl -> 0; // leaf
>>   case ValueLayout vl -> 0;
>> };
>> var v2 = switch (memoryLayout) {
>>   case PaddingLayout pl -> 0; // leaf
>>   case SequenceLayout sl -> 0; // leaf
>>   case ValueLayout vl -> 0;
>>   case StructLayout sl -> 0; // leaf
>>   case UnionLayout ul -> 0; // leaf
>> };
>> var v3 = switch (memoryLayout) {
>>   case PaddingLayout pl -> 0; // leaf
>>   case SequenceLayout sl -> 0; // leaf
>>   case StructLayout sl -> 0; // leaf
>>   case UnionLayout ul -> 0; // leaf
>>   case OfAddress oa -> 0; // leaf
>>   case OfBoolean ob -> 0; // leaf
>>   case OfByte ob -> 0; // leaf
>>   case OfChar oc -> 0; // leaf
>>   case OfDouble od -> 0; // leaf
>>   case OfFloat of -> 0; // leaf
>>   case OfInt oi -> 0; // leaf
>>   case OfLong ol -> 0; // leaf
>>   case OfShort os -> 0; // leaf
>> };
>> var v4 = switch (memoryLayout) {
>>   case GroupLayout gl -> 0;
>>   case PaddingLayout pl -> 0; // leaf
>>   case SequenceLayout sl -> 0; // leaf
>>   case OfAddress oa -> 0; // leaf
>>   case OfBoolean ob -> 0; // leaf
>>   case OfByte ob -> 0; // leaf
>>   case OfChar oc -> 0; // leaf
>>   case OfDouble od -> 0; // leaf
>>   case OfFloat of -> 0; // leaf
>>   case OfInt oi -> 0; // leaf
>>   case OfLong ol -> 0; // leaf
>>   case OfShort os -> 0; // leaf
>> };
>> 
>> 
>> 
>>  Additional improvements can be made later in a separate PR, including removing/improving some of the internal abstract classes.
>
> Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits:
> 
>  - Merge foreign-memaccess+abi
>  - Tighten up finality and permittable classes
>  - Remove redundant method call
>  - Fix problems in static initializers
>  - Merge foreign-memaccess+abi
>  - Update after comments
>  - Update src/java.base/share/classes/java/lang/foreign/MemoryLayout.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Add tests and update return types
>  - Rework MemoryLayout Sealed Hierarchy

Very nice clean up. Just one comment I think needs to be addressed regarding unnecessary new public static factory methods, no need for another review for any updates to address this.

src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 162:

> 160:          * @param order the byte order to use
> 161:          */
> 162:         static OfBoolean of(ByteOrder order) {

I think we should drop these static methods, they don't seem that useful given one can build from the constant. Further, the naming is a little awkward with two "of" e.g. `ValueLayout.OfBoolean.of(...)`.

Let's use the concrete implementations when creating `JAVA_BOOLEAN` etc.

src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java line 61:

> 59:  * @since 19
> 60:  */
> 61: @PreviewFeature(feature = PreviewFeature.Feature.FOREIGN)

This is internal so we don't need to declare the annotation, same for enclosing classes.

-------------

Marked as reviewed by psandoz (Committer).

PR: https://git.openjdk.org/panama-foreign/pull/710