[foreign-memaccess] RFR 8224483: Split MemoryAddress into separate address/region abstractions

[Maurizio Cimadamore]

On 21/05/2019 17:51, Jorn Vernee wrote:
> Hi,
>
> Some comments:
>
> MemoryAddress.java
> - I'm wondering if it makes sense to move the ofByteBuffer method to 
> MemorySegment? (This would also mirror the internal impl)
Was thinking that too - probably it makes sense, yes.
>
> MemoryAddressImpl.java
> - `((MemoryScopeImpl) segment().scope()).checkAlive()` is used in a 
> couple of places. Maybe move this to MemorySegmentImpl, and then call 
> segment.checkAlive()?
ok
> - I think asDirectByteBuffer needs to do a liveness check as well 
> (should probably just call checkAccess?).
yep - this part is untested as of yet (as we need to do more validation) 
but I agree.
>
> MemorySegmentImpl.java
> - OfEverything is unused and could be removed at this point.
I thought I did... probably forgot to do the last bit :-)
> - `resize` needs to check for negative offset as well, since we're 
> coming directly from user code. I think this check could be replaced 
> with a call to checkRange (adding a < 0 check for the length there).

Good point.


Thanks
Maurizio

>
> Cheers,
> Jorn
>
> Maurizio Cimadamore schreef op 2019-05-21 13:52:
>> Following discussion at [1], I decided to go ahead and split
>> MemoryAddres into two abstractions:
>>
>> * MemroyAddress will now embody an offest into a...
>> * MemorySegment, which represent a contiguous region of memory
>>
>> The name MemorySegment came from an internal discussion with Brian,
>> where he pointed out that MemoryScope and MemoryRegion where a bit too
>> overlapping, in the english sense of the word. MemorySegment suggests
>> something that has lower/upper boundaries.
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8224483/
>>
>> There are many changes since the last patch discussed - mostly
>> because, as I was adding the lower-level allocate methods in
>> MemoryScope I realized that we were missing a lot of checks (w.r.t.
>> alignment and sizes). I've added those and added tests for these as
>> well (2 new tests have been added). Also, the Scope::allocate API
>> should reflect the fact that Unsafe might fail to allocate - again
>> added a test for this.
>>
>> Maurizio