[foreign] RFR 8218153: Read/Write of Value layout should honor declared endianness

[Maurizio Cimadamore]

Thanks, some comments, again on withEndianness:

1) There's a missing override on Unresolved - this means that if the 
user calls withEndianness on Unresolved he'll get back a Layout, so you 
will lose type info

2) Group::withEndianness is weird:

if (elementLayout instanceof Value || elementLayout instanceof Group) {


With this you are basically covering the entire space, but Unresolved. I 
think it's better just to let virtual dispatch do its job - e.g. call 
withEndianness on the element. I wouldn't bother too much about 
minimizing the instances being created. If concrete benchmarks reveal an 
issue, we can fix it when the time comes. But in general layouts are 
only created once and then used multiple times; minimizing layout 
creation time is a second order issue, I think.

3) Same for Sequence

4) Address - should it ever have endianness? Yes. This is a deliberate 
choice; address layouts are more than just machine addresses - they will 
also be used to encode references to other parts of a message protocol 
schema - so in that case endianness could be relevant (as size).

The rest looks ok.

Maurizio

On 28/02/2019 20:13, Henry Jen wrote:
> OK, here is another update[1], and hopefully this is the last for basic support.
>
> As the jextract part, I have a preliminary version[2] we can review. The fix modify the Runner to get explicit endian layout instead of modifying the template. This should be enough, but I suspect this is not 100% fit to the philosophy to compare against golden master file.
>
> With jextract fix, future work to enforce check of explicit endianness on memory access can be continued, which I then expect issues to be fixed in UniversalInvoker.
>
> Cheers,
> Henry
>
>
> [1] http://cr.openjdk.java.net/~henryjen/panama/8218153/3/webrev/
> [2] http://cr.openjdk.java.net/~henryjen/panama/8218153/jextract/webrev/
>
>
>> On Feb 27, 2019, at 2:58 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>> Yeah - covering Group (Sequence is a Group) and Value (Address is a Value) gives you a lot of cover... but...
>>
>> Most of the code will work on 'Layout' - and if the root doesn't have the API point, clients will suffer.
>>
>> Actually, it seems to me that the only Layout for which this will be a no-op is Unresolved. Everything else will have a non-trivial override, as your current implementation of the protected method in Group demonstrates.
>>
>> Maurizio
>>
>> On 27/02/2019 22:25, Henry Jen wrote:
>>> OK, if we agreed that certainly can be done. :)
>>>
>>> Just that, the method doesn’t make sense for other Layout except Value and Group, so they will be no-op.
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>>> On Feb 27, 2019, at 2:13 PM, John Rose <john.r.rose at oracle.com> wrote:
>>>>
>>>> On Feb 27, 2019, at 11:06 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>> Henry I think what we were suggesting was to add a new interface method to Layout
>>>>>
>>>>> e.g.
>>>>>
>>>>> interface Layout
>>>>>
>>>>>     Layout withEndianness(Endianness)
>>>>> }
>>>>>
>>>>> and have the subtypes override accordingly. We want this to be usable by clients.
>>>> Yes please; that's what I was hoping for also.
>>>>
>>>>