RFR: 8237359: Clean up Binding
Jorn Vernee
jvernee at openjdk.java.net
Mon Feb 10 15:25:41 UTC 2020
On Mon, 10 Feb 2020 15:10:05 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:
>> Comment from @mcimadamore on the old mail thread:
>>> > - Constructing a COPY_BUFFER binding now requires a MemoryLayout, instead of a separate size & alignment. This was easier given the use-cases.
>>>
>>> Uhmmm - I'd agree with using a layoutm but patch says this:
>>>
>>> ```
>>> public AllocateBuffer(MemoryLayout layout) {
>>> + private Allocate(long size, long alignment) {
>>> super(ALLOC_BUFFER_TAG);
>>> - this.size = layout.byteSize();
>>> - this.alignment = layout.byteAlignment();
>>> + this.size = size;
>>> + this.alignment = alignment;
>>> }
>>> ```
>>>
>>> Seems like you went opposite way - e.g. from layout to size/alignment?
>>
>> I like to keep my constuctors more 1-to-1 with the fields of the class, and handle any argument conversions in a static factory method, so that is what I've done here. The factory methods `allocate` and `copy` do take a `MemoryLayout`
>
> Comment from @mcimadamore on the old mail thread:
>
>> I find the fact that Copy is also allocating a bit disturbing. I think copy should just mean that - e.g. copy from source address into target address. You can use Binding.allocate to allocate the target segment and Binding.BaseAddress to get the addresses. I think we should try to boil things down to primitive moves, so as to maximize reuse/composability.
>
> I agree with the sentiment, but after looking into it, it complicates the IR further since we need to shuffle more than 2 values around on the operand stack. Something that would require either load/store operators and a local variable table, or a swap operator.
>
> I'd like to investigate further and save that move for another time.
I've also added the suggested builder (see Binding.Builder), but kept the static factories since they were being used in the tests to declare the expected binding recipe.
-------------
PR: https://git.openjdk.java.net/panama-foreign/pull/12
More information about the panama-dev
mailing list