abstract layouts

[Ty Young]

I'd very much like it if layouts were opened up and non-sealed. It would 
allow for less wasteful(no Optional) and faster code(no HashMap, things 
can be final). It also would be nice if GroupLayout was removed and 
replaced with StructLayout and UnionLayout, or used as a shared/common 
interface/class for them.


I feel like the with* issues could be fixed with generics, e.g.:


public interface MemoryLayout<L extends MemoryLayout<?>> {

     public L withName(String name);

}


If SequenceLayout also used generics it would fix SequenceLayout not 
properly representing the element type, as you can then do:


SequenceLayout<ValueLayout> intSequence;


with SequenceLayout's class being:


public class SequenceLayout<L extends MemoryLayout<?>> implements 
MemoryLayout<SequenceLayout<L>>


All of the basic layout types should be interfaces, not classes IMO. 
Basic implementations could be put in a non-exported package. Users can 
get a basic implementation from MemoryLayout static methods, as they do now.


I don't think what I'm proposing could be done with static decorator 
methods. The information being added as attributes include MethodHandles 
for the class's constructor, e.g. a constructor MethodHandle for a 
NativeInteger class, something that specific can only be done at the 
class level I think.



On 8/12/21 5:08 AM, Maurizio Cimadamore wrote:

> 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
>>