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

[Jorn Vernee]

Comments inline...

Maurizio Cimadamore schreef op 2018-11-19 15:05:
> On 19/11/2018 13:34, Jorn Vernee wrote:
>>> 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).
>> 
>> Acutally, I realized this statement is based on the old 
>> BoundedMemoryRegion code (which I recently changed). Currently, the 
>> elements() method will just throw an UOE when calling it on a 
>> native-allocated pointer, because the underlying BoundedMemoryRegion 
>> has no explicit length. Meaning you can only iterate over pointers 
>> allocated from the Java side, which are all probably coming from 
>> `Array::elementPointer` any ways, so that's not a problem.
>> 
>> Creating a Stream manually using Stream::iterate and Pointer::offset 
>> still works for both though.
> 
> I see where you are coming from.
> 
> That said, if we applied your suggestion we would optimize for the
> array API case, which I'm not sure that is the most frequent case in
> API code (e.g. a lot of code doing iteration in native is doing so
> using simply pointers, not arrays).
> 
> I think I like that you can call Stream.iterate as a fallback and do
> the work. Let's see if there's a better restacking of the story here.
> 
> Now, Stream.iterate needs 3 parameters in the most general case: (i)
> the start, (ii) the termination condition and (iii) the increment
> step.
> 
> We can observe how in both the Array and the Pointer case we know (i)
> quite well, and we also happen to know (iii). This call for an API
> point which is something like that:
> 
> Stream<Pointer<T>> elements(Predicate<? super Predicate<T>> hasNext)

Yes, this is a nice idea. Maybe we'd also want to have an overload that 
just takes a `Pointer<T>` as an argument, and iterations will be up 
until that pointer.

> And this would generally applicable to both arrays and pointers. No
> guesswork involved, users iterating on pointers will have to provide
> an explicit termination condition; if they get it wrong, they will get
> some exception when dereferencing the wrong memory location.

Well, for native-allocated pointers this will invoke undefined behavior 
or possibly crash the VM (or enable access to VM internals? I'm not sure 
what security is in place there currently).

> For arrays, we can do more: since we have an explicit size, we also
> happen to know (ii) - that is, iterate until we have seen all array
> elements. This calls for an API like this:
> 
> Stream<Pointer<T>> elements()
> 
> I think this covers the design space nicely. Questions left are:
> 
> * should Array have both variants of the API? My preference would be
> to have only the no-arg variant - and use elementPointer() as a
> fallback there

Agreed.

> * what if the user wants to e.g. use a different step (e.g. increment
> by two elements instead of by one)? I'd say fallback on
> Stream.iterate/forloop for that.

Agreed.

> As a final point, I think we should implement elements() using
> Stream.iterate (and possibly document it in the API).

Agreed. Maybe we should also rename Pointer::elements to 
Pointer::iterate ?

Jorn