[foreign-memaccess] RFR 8234337: Misc foreign memory acess API cleanups

[Maurizio Cimadamore]

On 18/11/2019 14:46, Jorn Vernee wrote:
> Hi,
>
> Some comments:
> - MemoryLayout javadoc says "Non-platform classes should not implement 
> {@linkplain MemorySegment} directly." I think that should say 
> MemoryLayout?
I'll fix
> - MemoryLayout javadoc has no @apiNote about this potentially becoming 
> a sealed type, is this missing or is it intended like that?

I see that:

  * @apiNote In the future, if the Java language permits, {@link 
MemoryLayout}
  * may become a {@code sealed} interface, which would prohibit 
subclassing except by
  * explicitly permitted types.

> - MemorySegment javadoc says "Non-platform classes should not 
> implement {@linkplain MemoryLayout} directly." I think that should say 
> MemorySegment? ;)
Damn, will fix
> - MemorySegment::toByteArray javadoc: "@return a fresh byte array view 
> of this memory segment.". I think "view" is the wrong term here, since 
> it's a copy? How about "@return a fresh byte array copy of this memory 
> segment."?
Fair enough
> - In MemorySegment, several methods can throw an IllegalStateException 
> because of a call to checkState, this can happen either because the 
> segment was close, _or_ because the segment is being accessed by the 
> wrong thread. This should also be properly documented for those 
> method: asSlice, asReadOnly, close, toByteBuffer, and toByteArray (the 
> last 2 only document the throw in case of the segment already having 
> been closed).
Noted that when I was preparing the changes - are you implying I should 
add IllegalStateException where it applies? I can do that of course
> - In MemorySegmentImpl: now that we only care about readonly for 
> modes, this could use a boolean instead of the bit mask.
Well, yes. I believe a bit mask will come back handy at some point, 
which is why I've left the code as is.
> - javadoc of MS::allocateNative(MemoryLayout), 
> MS::allocateNative(long), and the package-info still mention `ofNative`.
Good catch.
> - TestArrays: since the point of this test is to test `toByteArray`, 
> I'm not sure it's needed to test with all the primitive types, since 
> each one ends up using the same code path for toByteArray? Maybe you 
> can simplify the testing, and move these tests to TestSegments.java?
Well, there is a bunch of stuff that gets tested - for instance, the 
test checks that if you have an int sequence layout, the byte array you 
get back is 4 * number of elements in the layout. So I think overall 
it's a bit on the paranoid side, but good to have.
>
> - Misc: I noticed SequenceLayout uses "elementsCount" as name for the 
> elemCount accessor. I think that should be "elementCount" (without the 
> 's').

Ok, will replace

Maurizio

>
> Jorn
>
> On 18/11/2019 14:05, Maurizio Cimadamore wrote:
>> Hi,
>> quoting from the JBS issues, there are a number of issues in the API 
>> which require rectification:
>>
>> * all views should use uniform naming (e.g. asXYZ)
>> * projections should use different naming (e.g. toByteBuffer)
>> * a projection from segment to byte array would be desirable
>> * segment factories should make it explicit as to whether allocation 
>> should be explicit
>> * segment pinning is mostly redundant now that we have acquire, and 
>> should be dropped (also, asPinned doesn't play nicely with clients 
>> using try with resources)
>> * the javadocs for MemoryLayout, MemorySegment, MemoryAddress and 
>> PathElement should be stricter about their immutability, sealedness 
>> and safety guarantees
>>
>> This webrev addresses the issues above:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234337/
>>
>> And an updated javadoc associated with these changes is available here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234337_javadoc/
>>
>> Main changes:
>>
>> * all views (whose temporal bound is shared with original segment) 
>> are called asXYZ (asSlice, asReadOnly)
>> * asPinned/isPinned have been dropped
>> * asByteBuffer has been renamed to 'toByteBuffer', and a new 
>> 'toByteArray' has been added (with a new test)
>> * MemorySegment::ofNative renamed to 'allocateNative'; 
>> MemorySegment::ofPath renamed to 'mapFromPath'
>> * misc javadoc improvements
>>
>> I think that now, if you look at instance methods in MemorySegments, 
>> the API looks pretty tight and consistent naming-wise.
>>
>> Cheers
>> Maurizio
>>