[foreign] RFR : Move array-like methods from Pointer to Array

[Jorn Vernee]

Maurizio Cimadamore schreef op 2018-11-19 12:46:
> I generally like the cleanup except for one thing: I believe
> restricting elements() for Array only is overly restrictive. When we
> added that method, the rationale was that there are a number of
> different iteration styles in native code which we wanna mimic with
> this API:
> 
> * iterate for N elements

This is possible by converting to the pointer to an Array.

> * iterate until the pointer is equal to some sentinel pointer

This is possible using `Pointer::offset`.

> * iterate until some condition is met (this could be seen as a
> generalization of the one above)

This is also possible using `Pointer::offset`.

> I think that, while the first case lends well to arrays, the remaining
> do not, as I'm not sure the client code always knows the 'size' to be
> given to the pointer.
> 
> So, it feels like an elements() method that returns a stream of
> pointers is an iteration primitive that belongs to pointers.

The reason I don't like the current approach of elements() is that the 
returned stream is essentially infinite, but it creates the false 
illusion that if you have a pointer to some native array (not obtained 
through Array::elementPointer, which is sized explicitly) that this 
stream will contain only the elements of that array, while in reality it 
will just keep iterating beyond that if the BoundedMemoryRegion is 
larger (which it _always_ is for arrays allocated on the native side).

I just think it's a pitfall currently, and I'm not sure that it's really 
needed.

You can still do the idiomatic pointer iteration loop using 
Pointer::offset:

     Pointer<Byte> start = getStart();
     Pointer<Byte> end = getEnd();
     for(var cur = start; !cur.equals(end); cur = cur.offset(1)) { // or 
-1
         // use `cur`
     }

If somebody wanted to, they could create a Stream manually using:

     Stream.iterate(getStart(), cur -> !cur.equals(getEnd()), cur -> 
cur.offset(1));

The current documentation of elements() says: "The size of the stream 
depends on the layout from which this array was obtained." But AFAICT 
this is only true for pointers obtained from `Array::elementPointer`. I 
think if we want to have an elements() method on Pointer we should 
clearly document that the size of the stream is undefined by default, 
and the user should take care to size the stream manually.

What do you think?

Jorn

> Maurizio
> 
> 
> On 17/11/2018 21:46, Jorn Vernee wrote:
>> Hi,
>> 
>> I'd like to contribute a patch that moves some of the array-like 
>> methods from Pointer to Array.
>> 
>> This patch moves the methods `long bytesSize()` and 
>> `Stream<Pointer<X>> elements()` from Pointer to Array, and removes the 
>> `long elementSize()` method (since Array already has `long length()` 
>> for that). The rationale for doing this is that these methods imply an 
>> inherent size associated with a Pointer, but this is not really the 
>> case, since we can only infer the size of memory regions that are 
>> allocated from the Java side. Array on the other hand has an explicit 
>> size associated with it, so I believe these methods fit a lot better 
>> in the Array interface.
>> 
>> The other thing is that `long bytesSize()` sounds like it returns the 
>> size of the pointee type, but this is not the case, as it actually 
>> returns the size of the underlying memory region minus the offset of 
>> the pointer, which can be much larger. I ran into some cases where 
>> `pointer.bytesSize()` was being used in error instead of 
>> `pointer.type().bytesSize()`, and I've made that mistake myself as 
>> well. So I think this move also removes that pitfall.
>> 
>> This change also actually simplified most use cases for the 
>> `bytesSize()` method, since they were already using an array I cloud 
>> simplify `array.elementPointer().bytesSize()` to `array.bytesSize()`. 
>> `elements()` was not being used at all in the JDK code.
>> 
>> Webrev : 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/arrays/webrev.03/
>> 
>> In this patch I've also:
>> - Updated the Javadoc of the Pointer class.
>> - Removed the `length()` method from BoundedMemoryRegion. Since it was 
>> no longer used, and it was only partially supported, I felt it was 
>> cleaner to remove it (at least for now).
>> - Simplified some of the `pointer.type().layout().bitSize() / 8` 
>> patterns to `pointer.type().bytesSize()`.
>> 
>> Cheers,
>> Jorn
>>