[foreign-memaccess+abi] RFR: 8270376: Finalize API for memory copy [v8]

[Paul Sandoz]

On Fri, 6 Aug 2021 14:43:15 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch ties up some loose ends with the MemoryCopy API, and it also prepares the ground for some other related refactorings in this area.
>> 
>> The meat of this patch is represented by the various changes in MemoryCopy, where all methods were renamed to simply `copy`, and the length parameter (now called `elementCount`) is always moved to the end.
>> 
>> I've also simplified naming of parameters, as I think distinguishing between `index` and `offset` is enough (e.g. an array has an index, a segment has an offset).
>> 
>> You will see that, at the very end of the class, three more copy methods have been added, which deal fully in terms of segments. I have also moved the complex layout-based memory segment copy operation (which does the swap) as a static method in this class, as I believe the static form makes the method more regular and usable.
>> 
>> I've made some changes to our uses of `copyFrom` in the linker, to use the new methods in `MemoryCopy` when the copy operation was performing slicing in the source/target segment, which I think makes the code more readable. Of course these changes are completely optional and could be omitted as well.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Switch order of bulk write method parameters (so that segment is always first).

I was not following the whole discussion too closely, so I hope I am not going over old ground, but this is my impression of the latest patch:

- I can understand the appeal of consistently using the read/write prefixes, but I think it may be confusing. In our Java APIs these names are commonly used for serialization and input/output streams etc. For memory access it would be more consistent to use set/get for individual access and copy for bulk access, rather than biasing towards whether the memory segment is the src or dst. The location in `MemoryAccess` is fine. Overloading with `copy` is probably fine, rather than identifying in the name whether the array is src or dst.

- For the copy methods I would prefer consistency with `System.arraycopy`, where the src occurs first regardless of the type (rather than segment src or dst, and the src flipping argument position). It’s really easy to make stupid mistakes confusing src/dst and I think this could increase such mistakes.

- Expanding on the latter point arguably suggests that `MemorySegment.copyFrom` should be changed to `MemorySegment.copyInto` (or just `copy`), so the receiver is the src, and it returns the destination. Then there is the same consistency of arguments as in the other copy methods. However, that would bury the dst segment allocation as an argument rather than as the receiver, but at least i would be able to reason consistently about the src/dst offsets/lengths argument positions.

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

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