Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 18:44:18 GMT, Daniel Fuchs  wrote:

>> I think the method is called during module bootstrap - I don't think there 
>> is a race in practice. This method is also called in other parts of 
>> ModuleBootstrap. The code you allude to is called during initialization of 
>> the IllegalNativeAccessChecker singleton, which should happen only once, and 
>> only from one thread.
>
> I'll take your word for it - the use of a volatile variable to store the 
> singleton instance made this suspicious.

good point - I think that came in when I adapted the code from 
IllegalAccessLogger - which can indeed be accessed from multiple threads. In 
this case the initialization is effectively part of bootstrap, and volatile is 
not required.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 19:02:57 GMT, Daniel Fuchs  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
>  line 209:
> 
>> 207: /**
>> 208:  * The native memory address instance modelling the {@code NULL} 
>> address, associated
>> 209:  * with the {@link ResourceScope#globalScope() global} resource 
>> scope.
> 
> `{@linkplain }` ?

thanks - I'll do a pass - I think I probably got all of them wrong

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Daniel Fuchs
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/LibraryLookup.java
 line 52:

> 50:  * 
> 51:  * For {@link #lookup(String) memory addresses} obtained from a library 
> lookup object,
> 52:  * since {@link CLinker#downcallHandle(Addressable, MethodType, 
> FunctionDescriptor) native method handles}

These should be `{@linkplain }` since the text of the link is plain text (and 
not code)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/LibraryLookup.java
 line 88:

> 86:  * @return the memory segment associated with the library symbol (if 
> any).
> 87:  * @throws IllegalArgumentException if the address associated with 
> the lookup symbol do not match the
> 88:  * {@link MemoryLayout#byteAlignment() alignment constraints} in 
> {@code layout}.

Same remark here (`{@linkplain }`)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
 line 43:

> 41:  * when performing memory dereference operations using a memory access 
> var handle (see {@link MemoryHandles}).
> 42:  * 
> 43:  * A memory address is associated with a {@link ResourceScope resource 
> scope}; the resource scope determines the

`{@linkplain }`

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
 line 46:

> 44:  * lifecycle of the memory address, and whether the address can be used 
> from multiple threads. Memory addresses
> 45:  * obtained from {@link #ofLong(long) numeric values}, or from native 
> code, are associated with the
> 46:  * {@link ResourceScope#globalScope() global resource scope}. Memory 
> addresses obtained from segments

... and here to (`{@linkplain }`)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
 line 102:

> 100:  * @param segment the segment relative to which this address offset 
> should be computed
> 101:  * @throws IllegalArgumentException if {@code segment} is not 
> compatible with this address; this can happen, for instance,
> 102:  * when {@code segment} models an heap memory region, while this 
> address is a {@link #isNative() native} address.

`{@linkplain }`

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
 line 209:

> 207: /**
> 208:  * The native memory address instance modelling the {@code NULL} 
> address, associated
> 209:  * with the {@link ResourceScope#globalScope() global} resource 
> scope.

`{@linkplain }` ?

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Daniel Fuchs
On Wed, 28 Apr 2021 18:19:14 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java
>>  line 78:
>> 
>>> 76: int index = 0;
>>> 77: // the system property is removed after decoding
>>> 78: String value = getAndRemoveProperty(prefix + index);
>> 
>> I am not sure what is going on with the removal of the properties, but if 
>> I'm not mistaken this is racy: from the implementation of the checker() 
>> method above, it looks as if two different threads could trigger a call to 
>> the decode() function concurrently, which can result in a random 
>> partitioning of the properties against the two checkers being instantiated, 
>> with one of them being eventually set as the system-wide checker.
>
> I think the method is called during module bootstrap - I don't think there is 
> a race in practice. This method is also called in other parts of 
> ModuleBootstrap. The code you allude to is called during initialization of 
> the IllegalNativeAccessChecker singleton, which should happen only once, and 
> only from one thread.

I'll take your word for it - the use of a volatile variable to store the 
singleton instance made this suspicious.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Mandy Chung
On Wed, 28 Apr 2021 18:26:30 GMT, Maurizio Cimadamore  
wrote:

> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java

Yes that's the test.  That test misses to catch your case where aliasing may be 
possible.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 17:53:44 GMT, Daniel Fuchs  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java
>  line 40:
> 
>> 38: 
>> 39: private IllegalNativeAccessChecker(Set allowedModuleNames, 
>> boolean allowAllUnnamedModules) {
>> 40: this.allowedModuleNames = 
>> Collections.unmodifiableSet(allowedModuleNames);
> 
> Should that be Set.copyOf() to take advantage of the immutability of `SetN` 
> (but at the expense of additional copying)...

Honestly, I'm not even sure why we should be concerned about mutation of the 
set here - since we create this with a bunch of values read from a system 
property... e.g. should we just store `allowedModuleNames` period?

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 18:23:41 GMT, Mandy Chung  wrote:

> > I believe that we had to move @CallerSensitive out of interfaces because 
> > there was a test that was checking that @cs was not put on "virtual" 
> > methods.
> 
> Good if we do have such test. We need to re-examine this with the security 
> team. I suggest to create a JBS issue and follow up separately.

The test in question is:

test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Mandy Chung
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

> I believe that we had to move @CallerSensitive out of interfaces because 
> there was a test that was checking that @cs was not put on "virtual" methods.

Good if we do have such test.We need to re-examine this with the security 
team.   I suggest to create a JBS issue and follow up separately.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 18:07:32 GMT, Daniel Fuchs  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java
>  line 78:
> 
>> 76: int index = 0;
>> 77: // the system property is removed after decoding
>> 78: String value = getAndRemoveProperty(prefix + index);
> 
> I am not sure what is going on with the removal of the properties, but if I'm 
> not mistaken this is racy: from the implementation of the checker() method 
> above, it looks as if two different threads could trigger a call to the 
> decode() function concurrently, which can result in a random partitioning of 
> the properties against the two checkers being instantiated, with one of them 
> being eventually set as the system-wide checker.

I think the method is called during module bootstrap - I don't think there is a 
race in practice. This method is also called in other parts of ModuleBootstrap. 
The code you allude to is called during initialization of the 
IllegalNativeAccessChecker singleton, which should happen only once, and only 
from one thread.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 18:02:03 GMT, Mandy Chung  wrote:

> I reviewed the `--enable-native-access` related change that looks fine.
> 
> > Access to restricted methods from any other module not in the list is 
> > disallowed and will result in an IllegalAccessException.
> 
> I think you meant to say `IllegalCallerException` instead of 
> `IllegalAccessException`. Also do you intend to have javadoc to generate 
> `@throw IllegalCallerException` for the restricted methods automatically 
> besides the javadoc description?
> 

IllegalCalller is probably better yes - we started off with an access-like 
check, so things have evolved a bit. I'll also add the @throws.

> Making the restricted methods as `@CallerSensitive` in order to get the 
> caller class for native access check is the proper approach. However, some 
> interface methods are restricted methods such as `CLinker::downcallHandle` 
> whose the implementation method is `@CallerSensitive`. I concern with the 
> security issue with method handle and type aliasing. On the other hand, 
> `CLinker` is a sealed interface and only implemented by the platform and so 
> it's less of a concern. I think the interface method should also be 
> `@CallerSensitive` so that for example a method handle for 
> `CLinker::downcallHandle` will be produced with the proper caller-sensitive 
> context.

I believe that we had to move @CallerSensitive out of interfaces because there 
was a test that was checking that @CS was not put on "virtual" methods.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Daniel Fuchs
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java 
line 40:

> 38: 
> 39: private IllegalNativeAccessChecker(Set allowedModuleNames, 
> boolean allowAllUnnamedModules) {
> 40: this.allowedModuleNames = 
> Collections.unmodifiableSet(allowedModuleNames);

Should that be Set.copyOf() to take advantage of the immutability of `SetN` 
(but at the expense of additional copying)...

src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java 
line 78:

> 76: int index = 0;
> 77: // the system property is removed after decoding
> 78: String value = getAndRemoveProperty(prefix + index);

I am not sure what is going on with the removal of the properties, but if I'm 
not mistaken this is racy: from the implementation of the checker() method 
above, it looks as if two different threads could trigger a call to the 
decode() function concurrently, which can result in a random partitioning of 
the properties against the two checkers being instantiated, with one of them 
being eventually set as the system-wide checker.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Mandy Chung
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

I reviewed the `--enable-native-access` related change that looks fine.

> Access to restricted methods from any other module not in the list is 
> disallowed and will result in an IllegalAccessException.

I think you meant to say `IllegalCallerException` instead of 
`IllegalAccessException`.  Also do you intend to have javadoc to generate 
`@throw IllegalCallerException` for  the restricted methods automatically 
besides the javadoc description?

Making the restricted methods as `@CallerSensitive` in order to get the caller 
class for native access check is the proper approach.   However, some interface 
methods are restricted methods such as `CLinker::downcallHandle` whose the 
implementation method is `@CallerSensitive`.I concern with the security 
issue with method handle and type aliasing.   On the other hand, `CLinker` is a 
sealed interface and only implemented by the platform and so it's less of a 
concern.   I think the interface method should also be `@CallerSensitive` so 
that for example a method handle for `CLinker::downcallHandle` will be produced 
with the proper caller-sensitive context.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Daniel Fuchs
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/package-info.java 
line 224:

> 222:  * Some methods in this package are considered restricted. 
> Restricted methods are typically used to bind native
> 223:  * foreign data and/or functions to first-class Java API elements which 
> can then be used directly by client. For instance
> 224:  * the restricted method {@link 
> jdk.incubator.foreign.MemoryAddress#asSegment(long, ResourceScope)} can be 
> used to create

typo: `used directly by client.` => `used directly by clients.` ?

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Alan Bateman
On Wed, 28 Apr 2021 13:47:43 GMT, Maurizio Cimadamore  
wrote:

>> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java 
>> line 270:
>> 
>>> 268: 
>>> 269: /**
>>> 270:  * Converts a Java string into a null-terminated C string, using 
>>> the platform's default charset,
>> 
>> Sorry if this has come up before, but, is the platform's default charset the 
>> right choice here? For other areas, we choose UTF-8 as the default. In fact, 
>> there is a draft JEP to move the default charset to UTF-8. So if there is an 
>> implicit need to match the underlying platform's charset then this may need 
>> to be revisited.  For now, I just want to check that this is not an 
>> accidental reliance on the platform's default charset, but a deliberate one.
>
> I believe here the goal is to be consistent with `String::getBytes`:
> 
> 
> /**
>  * Encodes this {@code String} into a sequence of bytes using the
>  * platform's default charset, storing the result into a new byte array.
>  *
>  *  The behavior of this method when this string cannot be encoded in
>  * the default charset is unspecified.  The {@link
>  * java.nio.charset.CharsetEncoder} class should be used when more control
>  * over the encoding process is required.
>  *
>  * @return  The resultant byte array
>  *
>  * @since  1.1
>  */
> public byte[] getBytes() {
> return encode(Charset.defaultCharset(), coder(), value);
> }
> 
> 
> So, you are right in that there's an element of platform-dependency here - 
> but I think it's a dependency that users learned to "ignore" mostly. If 
> developers want to be precise, and platform independent, they can always use 
> the Charset-accepting method. Of course this could be revisited, but I think 
> there is some consistency in what the API is trying to do. If, in the future, 
> defaultCharset will just be Utf8 - well, that's ok too - as long as the 
> method is specified to be "defaultCharset" dependent, what's behind 
> "defaultCharset" is an implementation detail that the user will have to be 
> aware of one way or another.

Naoto is working on a couple of changes in advance of JEP 400. One of these is 
to expose a system property with the host charset and I suspect that the 
CLinker method will want to use that instead of Charset.defaultCharset.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 12:47:36 GMT, Chris Hegarty  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java 
> line 270:
> 
>> 268: 
>> 269: /**
>> 270:  * Converts a Java string into a null-terminated C string, using 
>> the platform's default charset,
> 
> Sorry if this has come up before, but, is the platform's default charset the 
> right choice here? For other areas, we choose UTF-8 as the default. In fact, 
> there is a draft JEP to move the default charset to UTF-8. So if there is an 
> implicit need to match the underlying platform's charset then this may need 
> to be revisited.  For now, I just want to check that this is not an 
> accidental reliance on the platform's default charset, but a deliberate one.

I believe here the goal is to be consistent with `String::getBytes`:


/**
 * Encodes this {@code String} into a sequence of bytes using the
 * platform's default charset, storing the result into a new byte array.
 *
 *  The behavior of this method when this string cannot be encoded in
 * the default charset is unspecified.  The {@link
 * java.nio.charset.CharsetEncoder} class should be used when more control
 * over the encoding process is required.
 *
 * @return  The resultant byte array
 *
 * @since  1.1
 */
public byte[] getBytes() {
return encode(Charset.defaultCharset(), coder(), value);
}


So, you are right in that there's an element of platform-dependency here - but 
I think it's a dependency that users learned to "ignore" mostly. If developers 
want to be precise, and platform independent, they can always use the 
Charset-accepting method. Of course this could be revisited, but I think there 
is some consistency in what the API is trying to do. If, in the future, 
defaultCharset will just be Utf8 - well, that's ok too - as long as the method 
is specified to be "defaultCharset" dependent, what's behind "defaultCharset" 
is an implementation detail that the user will have to be aware of one way or 
another.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 13:08:26 GMT, Chris Hegarty  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
>  line 693:
> 
>> 691:  */
>> 692: static MemorySegment allocateNative(MemoryLayout layout, 
>> ResourceScope scope) {
>> 693: Objects.requireNonNull(scope);
> 
> Should the allocateNative methods declare that they throw ISE, if the given 
> ResourceScope is not alive?   ( I found myself asking this q, then 
> considering the behaviour of a SegmentAllocator that is asked to allocate 
> after a RS has been closed )

Good point, yes it should

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Chris Hegarty
On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

Like Paul, I tracked the changes to this API in the Panama repo. Most of my 
remaining comments are small and come from re-reading the API docs.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 
270:

> 268: 
> 269: /**
> 270:  * Converts a Java string into a null-terminated C string, using the 
> platform's default charset,

Sorry if this has come up before, but, is the platform's default charset the 
right choice here? For other areas, we choose UTF-8 as the default. In fact, 
there is a draft JEP to move the default charset to UTF-8. So if there is an 
implicit need to match the underlying platform's charset then this may need to 
be revisited.  For now, I just want to check that this is not an accidental 
reliance on the platform's default charset, but a deliberate one.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 101:

> 99:  * Lifecycle and confinement
> 100:  *
> 101:  * Memory segments are associated to a resource scope (see {@link 
> ResourceScope}), which can be accessed using

Typo?? "Memory segments are associated *with* a resource scope"

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 112:

> 110: MemoryAccess.getLong(segment); // already closed!
> 111:  * }
> 112:  * Additionally, access to a memory segment in subject to the 
> thread-confinement checks enforced by the owning scope; that is,

Typo?? "Additionally, access to a memory segment *is* subject to ..."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 114:

> 112:  * Additionally, access to a memory segment in subject to the 
> thread-confinement checks enforced by the owning scope; that is,
> 113:  * if the segment is associated with a shared scope, it can be accessed 
> by multiple threads; if it is associated with a confined
> 114:  * scope, it can only be accessed by the thread which own the scope.

Typo?? "it can only be accessed by the thread which ownS the scope."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 693:

> 691:  */
> 692: static MemorySegment allocateNative(MemoryLayout layout, 
> ResourceScope scope) {
> 693: Objects.requireNonNull(scope);

Should the allocateNative methods declare that they throw ISE, if the given 
ResourceScope is not alive?   ( I found myself asking this q, then considering 
the behaviour of a SegmentAllocator that is asked to allocate after a RS has 
been closed )

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
On Wed, 28 Apr 2021 09:10:37 GMT, Chris Hegarty  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
>  line 135:
> 
>> 133: } finally {
>> 134:segment.scope().release(segmentHandle);
>> 135: }
> 
> I do like the symmetry in this example, but just to add an alternative idiom:
> `segmentHandle.scope().release(segmentHandle)`
> , which guarantees to avoid release throwing and IAE

I see what you mean - I don't think I want to encourage that style too much by 
giving it prominence in the javadoc. It's true you can go back to the scope 
from the handle, so that you are guaranteed to release the right thing, but I 
think that should be unnecessary in most idiomatic uses.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Chris Hegarty
On Wed, 28 Apr 2021 09:06:29 GMT, Chris Hegarty  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
>  line 205:
> 
>> 203:  * until all the resource scope handles acquired from it have been 
>> {@link #release(Handle)} released}.
>> 204:  * @return a resource scope handle.
>> 205:  */
> 
> Given recent changes, it might be good to generalise the opening sentence 
> here. Maybe :
>  " Acquires a resource scope handle associated with this resource scope. If 
> the resource scope is explicit ... "   Or some such.

The spec for the acquire method talks only of explicit resource scopes. The 
comment is suggesting that the spec could be generalised a little, since it is 
possible to acquire a resource scope handle associated with implicit scopes.

-

PR: https://git.openjdk.java.net/jdk/pull/3699


Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

2021-04-28 Thread Maurizio Cimadamore
> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Address first batch of review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3699/files
  - new: https://git.openjdk.java.net/jdk/pull/3699/files/7545f71f..a80d8180

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3699=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3699=00-01

  Stats: 42 lines in 4 files changed: 25 ins; 11 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3699.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699

PR: https://git.openjdk.java.net/jdk/pull/3699