abstract layouts

[Maurizio Cimadamore]

As we're approaching finalization of the API, I've been thinking about 
things that are suboptimal, or that can be pruned from the API. Layout 
attributes, frankly, seems one of such things.

I think it would generally be more useful to allow for inheritance, so 
that you can define your own e.g. ValueLayout with the extra info you 
need stapled onto the side.

One problem in the way of that is the fact that the layout API features 
some "wither" methods: for instance `MemoryLayout::withName(String 
string)`. Let's imagine a client adds a custom subclass of ValueLayout:

```
class ColoredLayout extends ValueLayout {
     enum Color {
          RED, GREEN, BLUE;
     }

     final Color color;

     ColoredLayout(Color color, ... ??) {
        ...
     }

     ...
}
```

Here, what happens if the client doesn't override `withName`, 
`withBitAlignment`, etc. ? Well, the `super` implementation will be 
called - but that implementation will yield a `ValueLayout` not a 
`ColoredLayout`!

Fixing this is difficult, as the Java language doesn't have the concept 
to mark a method as "you need to re-override this, even if an impl is 
available in the supertype".

One thing we could do to approach to this would be to insert an 
intermediate class:

```
class AbstractValueLayout extends ValueLayout { // extension point!
      abstract AbstractValueLayout withName(String name);
      ...
}
```

E.g. re-abstract all the wither methods, so that clients are forced to 
override them. But this approach has limitations too. For one, the 
hierarchy looks odd, as the abstract class is at the bottom - but maybe 
that can be fixed if we were willing to change the entire API to take 
AbstractValueLayout in place of ValueLayout - at which point ValueLayout 
would just be a leaf.

But the more worrying issue is that this trick would have to be repeated 
for every layout subclasses (e.g. group, sequence layouts) - so we end 
up with 3 extra abstract classes, which are used as extension points. 
That's a lot of API surface for something that's not very common to use 
(which is the same problem layout attributes have).


For these reasons, in the short term I'm tempted to just remove layout 
attributes, and either leave the hierarchy sealed (which would mean loss 
of functionality, albeit corner case one), or just open up the hierarchy 
and allow subclasses (maybe with some spelling in the javadoc which says 
"make sure to override the withers or it won't work").

P.S.

As to your original question, I think all you did is achievable using 
static methods on the side which take a layout and add the empty 
attributes. Layout attributes are (see above) a corner use case, and 
abstract attributes are even a more cornery one (although I don't doubt 
they are useful in your case), and I think adding the API you propose 
has very low return on complexity. The hope is that, if we do end up 
opening up the layout hierarchy (with some javadoc caveats), we'll be 
out of your hair, and you can add as many custom layouts as you want - 
which I think is a solution you would prefer in the long run.

Cheers
Maurizio


On 12/08/2021 10:23, Ty Young wrote:
> Hi,
>
>
> When decorating a layout with attributes, there is no way to declare 
> that some attributes need to be filled in by an API user. In order to 
> fix this, I'd like to introduce some simple changes in order to 
> support that[1]:
>
>
> 1. withAttribute no longer accepts a null value as an attribute value.
>
> 2. Addition of an asAbstract method[2], which takes in a var args of 
> attributes that need to be filled in(indicated by a null value).
>
> 3. Addition of an isAbstract method[3], which returns true if any 
> attribute has a value of null.
>
> 4. Addition of an unfilledAttributes method[4] which returns a list of 
> all attributes that have a null attribute value and need filled in.
>
>
> There are some minor code style consistency mistakes with the 
> asAbstract method and the docs could be improved, but this is more 
> about just putting the idea out there.
>
>
> [1] https://github.com/BlueGoliath/panama-foreign/tree/abstract_layouts
>
>
> [2] 
> https://github.com/BlueGoliath/panama-foreign/blob/a1c81e0b2b929cc3e1738c31acc97830294e0346/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java#L251
>
>
> [3] 
> https://github.com/BlueGoliath/panama-foreign/blob/a1c81e0b2b929cc3e1738c31acc97830294e0346/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java#L610
>
>
> [4] 
> https://github.com/BlueGoliath/panama-foreign/blob/a1c81e0b2b929cc3e1738c31acc97830294e0346/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java#L342
>