Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading

Mandy Chung mandy.chung at oracle.com
Tue Mar 17 23:09:36 UTC 2020


Hi Alan,

Thanks for the comment.  See my comments inlined below.

Here is the updated webrev:
   http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.01

On 3/16/20 3:47 AM, Alan Bateman wrote:
> The difference between the 2 constructors might not be obvious at the 
> use sites. I'm just wondering if would be better to use static factory 
> methods instead, e.g. the 2-arg constructor could be replaced with a 
> trusted(caller, searchLibPath) method that would make it a lot more 
> obvious in the places where that will eventually be used.
>

I have similar comment to myself and didn't come up good static factory 
method names.   I give it a try again: what about newNativeLibraries and 
newNativeLibrariesWithNoAutoUnload?

> A small inconsistency is that VM.isSystemDomainLoader is used in 
> constructor whereas the other checks for null and platform class 
> loader (plus app class loader).
>

Good catch.  Revised.

> The Main test could use Path.of("classes"). In setup, dir could be a 
> Path and also Path p = Files.createDirectories(...) would allow the 
> Files.move to be a bit more readable. 

Adjusted.

> I can't quite tell why the test is skipped with -Xcomp but maybe it's 
> just too slow and times out?
>

Removed. Cut-n-paste error from an existing test.

> A small suggestion for NativeLibrariesTest is that 
> loadWithCustomLoader might be a better name to load p.Test with a 
> custom loader. Also noticed libnativeLibrariesTest.c has a 2017 date 
> on it.

Fixed.

thanks
Mandy
>
> -Alan.



More information about the core-libs-dev mailing list