[foreign-memaccess] RFR 8234814: Eager layout size computation trips on unbound sequence layouts

[Jorn Vernee]

Hi,

Some comments:

- SequenceLayout:
This overrides badSizeExceptionMessage. I see the subtle difference in 
text, but a sequence layout can also have an unspecified size if the 
element layout has an unspecified size, in which case the message in 
AbstractLayout would be more correct. You could check whether the 
current sequence has an element count and return 
super.badSizeExceptionMessage() in that case, though I think going with 
one message that reads "Cannot compute the size of a layout which is, or 
depends on a sequence layout with unspecified size" in AbstractLayout 
would be good enough.

- Shouldn't the JShell fix should go into jdk/jdk instead?

One observation:

* Looking at the tests; I think something like 
`MemorySegment.allocateNative(8 + 8 * 4)` would in practice be more 
likely `MemorySegment.allocateNative(layout.byteSize() + 
MemoryLayout.ofSequence(4, MemoryLayouts.JAVA_DOUBLE).byteSize())` which 
makes me think we need a SequenceLayout::withElementCount(long) method 
to create a new layout with an element count from a sequence layout that 
doesn't have one:

         SequenceLayout vla = 
MemoryLayout.ofSequence(MemoryLayouts.JAVA_DOUBLE);
         MemoryLayout header = MemoryLayout.ofStruct(
                 MemoryLayouts.JAVA_INT.withName("size"),
                 MemoryLayout.ofPaddingBits(32),
                 vla.withName("arr"));
         ...
         try (MemorySegment segment = 
MemorySegment.allocateNative(header.byteSize() + 
vla.withElementCount(4).byteSize())) {
         ...

Especially when 'vla' in the above example could be a lot more complex. 
What do you think?

Cheers,
Jorn

On 26/11/2019 16:03, Maurizio Cimadamore wrote:
> After a discussion offline with Jorn I've decided to revert the 
> changes to constants:
>
> cr.openjdk.java.net/~mcimadamore/panama/8234814_v3
>
> As some of the tests demonstrate, among the original goals of these 
> JAVA_INT constants and friends was interop with ByteBuffers, hence the 
> endianness set to BE.
>
> I'll leave it as is for now (after all, the behavior is documented). 
> We can revise later, if needs be.
>
> Maurizio
>
> On 26/11/2019 14:46, Maurizio Cimadamore wrote:
>>
>> On 26/11/2019 14:40, Maurizio Cimadamore wrote:
>>> * constants such as JAVA_INT should use native order (this is, after 
>>> all, the layout of an int in the VM) 
>>
>> To expand a bit - we eventually would like to see these constants 
>> moved in the primitive wrapper classes - e.g.
>>
>> MemoryLayout::JAVA_INT -> Integer::LAYOUT
>>
>> in which case I think these constants should return whatever layout 
>> the VM thinks a Java int (in this case) has.
>>
>> Maurizio
>>