[foreign-jextract] RFR: 8263018: Improve API for lifecycle of native resources [v2]

[Maurizio Cimadamore]

On Wed, 24 Mar 2021 12:42:37 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 98 commits:
>> 
>>  - Merge branch 'foreign-jextract' into jextract+resourceScope
>>  - Merge branch 'foreign-memaccess+abi' into jextract+resourceScope
>>  - Merge branch 'foreign-jextract' into jextract+resourceScope
>>  - Add overloads accepting ResourceScope parameter
>>  - Merge branch 'resourceScope' into jextract+resourceScope
>>  - Add ResourceScope overloads in CLinker
>>    Rewrite StdLib test not to use NativeScope
>>    Tweak names of allocator factories
>>    Fix javadoc
>>  - All tests pass
>>    * Fix upcall overload taking NativeScope
>>    * Generate NativeScope in output package
>>    * Add overload for functions returning a struct so than a NativeScope can be passed
>>    * Removed some now unused features from RuntimeHelper (ti make scope non-transferrable)
>>    * Fixup vararg invoker to deal with allocator parameters
>>    * Fixup tests to (a) depend on the generated NativeScope and (b) not to use TWR with segments directly
>>    Notes:
>>    * the internal libclang probably needs to be re-generated
>>  - Merge branch 'resourceScope' into jextract+resourceScope
>>  - Fix javadoc of SegmentAllocator
>>  - Address rest of review comments from Paul
>>  - ... and 88 more: https://git.openjdk.java.net/panama-foreign/compare/4e696684...6316ddca
>
> src/jdk.incubator.jextract/share/classes/jdk/internal/clang/libclang/RuntimeHelper.java line 93:
> 
>> 91:                     return variadic ?
>> 92:                         VarargsInvoker.make(addr, mt, fdesc) :
>> 93:                         LINKER.downcallHandle(addr, DEFAULT_ALLOCATOR, mt, fdesc);
> 
> Using a managed segment by default here also seems like asking for trouble, since we don't know how the returned struct is going to be used (e.g. might be projected into an address).
> 
> Would seem more robust to have a throwing allocator used here for functions that don't have MS as a return type, and then determine on a case-by-case basis which scope to use for MS returns. (sorry)

Uhm.. in the new generated code this is not a problem - e.g. this is not what RuntimeHelper looks like in the new world. This is mostly a way to get libclang up and running. We could of course do what you suggest, I was unsure whether now is the time to do it (e.g. won't we regenerated libclang anyway?)

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/FunctionalInterfaceBuilder.java line 113:
> 
>> 111:                 delim = ", ";
>> 112:             }
>> 113:             append(") -> {\n");
> 
> Looks like this is also switching from anon classes to lambdas?

yes - I changed it since I was there

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/HeaderFileBuilder.java line 139:
> 
>> 137:             declType = declType.insertParameterTypes(0, SegmentAllocator.class);
>> 138:             paramNames = new ArrayList<>(paramNames);
>> 139:             paramNames.add("allocator");
> 
> Shouldn't this be added as the first element rather than the last?

uhm, yes

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/474