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

[Jorn Vernee]

Hi,

Some comments:
- MemoryLayout javadoc says "Non-platform classes should not implement 
{@linkplain MemorySegment} directly." I think that should say MemoryLayout?
- MemoryLayout javadoc has no @apiNote about this potentially becoming a 
sealed type, is this missing or is it intended like that?
- MemorySegment javadoc says "Non-platform classes should not implement 
{@linkplain MemoryLayout} directly." I think that should say 
MemorySegment? ;)
- 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."?
- 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).
- In MemorySegmentImpl: now that we only care about readonly for modes, 
this could use a boolean instead of the bit mask.
- javadoc of MS::allocateNative(MemoryLayout), MS::allocateNative(long), 
and the package-info still mention `ofNative`.
- 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?
- Misc: I noticed SequenceLayout uses "elementsCount" as name for the 
elemCount accessor. I think that should be "elementCount" (without the 's').

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
>