[foreign] RFR 8213343: Update the public access mode API

[Maurizio Cimadamore]

Hi Jorn,
the patch is sensible, of course, but I do have some comments, some 
general, some more specific.

 From a general perspective, I think in general it's better to keep the 
patches focussed on the issue at hand. If the problem here is to fix 
access mode, let's focus on that; I note that there's quite a bit of 
other refactoring going on, and that makes the review process harder.

In terms of design, I view all the read/write accesses as some kind of a 
placeholder for some story yet to materialize (and that is probably 
going to become clearer when we'll use this API in anger for other 
things such as ByteBuffers). That said, it is true that the constants 
are not exposed in the public API and that is, in itself an issue.

I can see two ways of solving this problem:

- adding an enum, as you did; or adding public static final constants

Then, I can also see few different ways to change access mode:

- with a single method which takes an EnumSet (in case we encode access 
as enum) or int mask (in case we encode access as ints, as now)
- with separate 'asRead', 'asWrite' methods, as you did. This assumes 
that we always start from a 'RW' state and the only possible choice is 
to restrict the access. Seems like a sensible assumption

Last, but more importantly, where does the access belong to:

- the pointer

- the region (what we have now)

- the scope

Having access on the pointer/region seems more flexible, but I also 
wonder how much people will take advantage of such flexibility. I 
believe in practice a scope will probably be created as 'morally 
read-only' from the start - e.g. all pointers allocated through it 
should be read-only. But we don't have enough hard evidence one way or 
another.

More specific comments on the code:

* the enum should be nested in the public Pointer API, as that's where 
it's used

* MODE_XYZ should be renamed to just R/W/RW (or READ/WRITE/READ_WRITE); 
after all the client code will do AccessMode... so repeating MODE in the 
constant name seems odd

* in BoundedArray::cast I wonder why we are not checking compatibility 
of the element type (not an issue introduced by your patch)

* The tests work fine for me even with the DIrectSignatureShuffler 
changes; that said, I think that change is suspicious because it relies 
on the pointer have same offset/size as the argument binding being 
passed as argument. So I would revert it anyway.

* we typically do not use packages in tests


Cheers
Maurizio


On 21/11/2018 02:35, Jorn Vernee wrote:
> Ok, I've found one more mistake.
>
> I made this patch a while ago already, and ran the tests to see if it 
> still worked. However I seem to have made a mistake in code that is 
> not being tested on windows (DirectSignatureShuffler). It seems 
> obvious now, but I guess I missed it a while back.
>
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.03/
>
> I've reverted the BoundedPointer fields back to being public. Making 
> them private wasn't as simple of a change as I thought.
>
> Sorry about that,
> Jorn
>
>
> Jorn Vernee schreef op 2018-11-21 00:22:
>> And I suppose it's useful if I include the actual AccessMode class in
>> the webrev as well (oops)
>>
>> Webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.01/
>>
>> Cheers,
>> Jorn
>>
>> Jorn Vernee schreef op 2018-11-21 00:14:
>>> Hi,
>>>
>>> While reading through the Pointer class, I noticed there is a method
>>> `isAccessibleFor(int)` which is used to check if the pointer is
>>> accessible for a certain access mode.
>>>
>>> The problem is that the `int` value that's expected as an argument is
>>> defined as the constants MODE_R, MODE_W and MODE_RW in
>>> BoundedMemoryRegion [1]. This is in an internal package, so there will
>>> be no way for the user to use those constants.
>>>
>>> The other problem is that having an `int` as the argument type gives
>>> no hint to what values can be used there. I would like to advocate for
>>> switching to an enum there, since then it's a simple case of typing in
>>> the enum name, hitting the auto-complete key and getting a list of
>>> available argument values.
>>>
>>> The third problem is that there seems to be no public API point for
>>> setting the access mode of a Pointer. I can imagine that somebody
>>> might want to give out a read-only pointer to a client (and maybe even
>>> a write-only pointer).
>>>
>>> I have a patch that attempts to solve all 3 problems;
>>>
>>> 1 & 2: I have created an AccessMode enum in j.f.memory that is used in
>>> place of the `int`s.
>>> 3: I have added 2 methods to Pointer, asReadOnly and asWriteOnly, to
>>> create a new pointer from the given one with either of the 2 access
>>> modes.
>>>
>>> As an additional cleanup I've also removed the mode field from
>>> BoundedPointer, since it was unused, and made the other fields private
>>> instead of public. In order to do the latter, I added
>>> putLongBits/getLongBits methods to BoundedPointer, which replaces
>>> direct access to the pointer's region put/get methods. I also added a
>>> package declaration to ArrayTest since my IDE (thanks Maurizio ;P) was
>>> complaining about a duplicate class, since there was a test class with
>>> the same name in the jextract test directory.
>>>
>>> Related bug: https://bugs.openjdk.java.net/browse/JDK-8213343
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.00/ 
>>>
>>>
>>> Please let me know what you think about this.
>>>
>>> Thanks,
>>> Jorn
>>>
>>> [1] :
>>> http://hg.openjdk.java.net/panama/dev/file/cfee9bc4ebe4/src/java.base/share/classes/jdk/internal/foreign/memory/BoundedMemoryRegion.java#l33 
>>>