RFR: 8237359: Clean up Binding

[Jorn Vernee]

On Mon, 10 Feb 2020 15:07:50 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Continuation of: https://mail.openjdk.java.net/pipermail/panama-dev/2020-February/007417.html
>> 
>>> Hi,
>>> 
>>> Please review the following patch that does a few cleanups of the 
>>> Binding class implementation:
>>> - Adds javadoc
>>> - Uses static factory methods instead of raw constructors. This allows 
>>> returning singletons for some of the operators for instance.
>>> - Constructing a COPY_BUFFER binding now requires a MemoryLayout, 
>>> instead of a separate size & alignment. This was easier given the use-cases.
>>> - Binding.AllocateBuffer renamed to Binding.Allocate (since we already 
>>> had Copy as well).
>>> - BoxAddress renamed to ConvertAddress, as a more general name to 
>>> describe the boxing & unboxing that can occur. Suggested by Maurizio.
>>> 
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/cleanup_bindings/webrev.00/
>>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8237359, 
>>> https://bugs.openjdk.java.net/browse/JDK-8238227, 
>>> https://bugs.openjdk.java.net/browse/JDK-8238235
>>> 
>>> This patch take care of several smaller issues all at once. It applies 
>>> on top of the JDK-8237360 patch.
>>> 
>>> Thanks,
>>> Jorn
> 
> 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.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/12