[foreign-abi] RFR: Clean up Binding

[Maurizio Cimadamore]

Looks good - some comments:

On 03/02/2020 15:15, Jorn Vernee wrote:
> 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.

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?
> - 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.

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.

Also, when I see all the calls to bindings.add... it is hard not to 
think to a builder - e.g. instead of:

List<Binding> bindings = new ArrayList<>();
bindings.add(move(storage, type));
bindings.add(dereference(offset, type));


Binding.Builder bindings = Binding.builder();
bindings.move(storage, type)
              .dereference(offset, type)

This will probably even avoid the need of having publicly exposed factories.

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
>