[foreign-memaccess] 8227393: Add Constable support to the layout API

[Maurizio Cimadamore]

Updated webrev:

http://cr.openjdk.java.net/~mcimadamore/panama/8227393_v2/

(addressed all comments, including covariant overrides)

Thanks
Maurizio

On 08/07/2019 16:22, Jorn Vernee wrote:
> Hi,
>
> Some comments:
>
> - The constants added to AbstractLayout should probably be `final`? 
> (Also minor naming; these use `XXX_CD` while ConstantDescs uses 
> `CD_XXX`, might want to follow the latter)
> - In MemoryHandles.java there are 2 imports added that are not used.
> - In TestLayoutConstants.java, there seems to be a spurious line: 
> `MemoryLayout.ofStruct(MemoryLayout.ofFloatingPoint(32), 
> MemoryLayout.ofPadding(8));`.
>
> Also, what do we want to do with covariant return types for various 
> `describeConstable` methods? e.g. GroupLayout::describeConstable could 
> return Optional<DynamicConstantDesc<GroupLayout>>, do we want to go 
> that route?
>
> Cheers,
> Jorn
>
> On 2019-07-08 16:17, Maurizio Cimadamore wrote:
>> As suggested by Brian, there's no reason as to why MemoryLayout
>> classes could not implement the Constable interface. After all, they
>> are static description of some memory layout, and a constant pool form
>> for them can be defined (using condy). This patch does just that.
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8227393/
>>
>> Now, there are some degree of freedom in doing this mapping; from
>> other cases, I infer that the idiomatic way to implement constable
>> would be to return a custom DynamicConstantDesc (e.g. enums return an
>> EnumDesc, VarHandles a VarHandleDesc) - but in this case I'm not sure
>> the exercise is worth it for two reasons:
>>
>> * I can't see any 'interesting method' that could be added to a
>> LayoutDesc - seems simpler to just work on the layouts themselves
>>
>> * To canonicalize dynamic constants representing layouts, we'd still
>> need some behavioral changes in the public SE API - which is a bit
>> tricky given everything is defined in an incubator module
>>
>> I'd say that what I have implemented is good enough for now, but of
>> course we should reserve the right to change it later if/when the code
>> ends up in java.base.
>>
>> Similar considerations hold for memory access VarHandles Constable
>> support - on the one hand we could just add such support - on the
>> other hand, to do that correctly we'd need some surgery in
>> VarHandleDesc - which is again a SE API in java.base. I think when the
>> time comes, we'd add a new 'kind' to VarHandleDesc called MEMORY, and
>> then add a suitable implementation of describeConstable under
>> VarHandleMemoryAddressBase (as well as canonicalize support).
>>
>> Note: I had to add a 'missing' BSM constant in ConstantDescs - namely
>> for ConstantBootstraps#getStaticFinal. I hope this change is
>> innocuous/standalone enough.
>>
>> Cheers
>> Maurizio