On Fri, 30 Apr 2021 15:20:42 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> 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 two > additional commits since the last revision: > > - Revert bad change in benchmark copyright > - Do not apply optimized bound check if accessed offset/length do not fit in > an `int` value Just to double, there is no way to enable native access for modules in module layers (other than the boot layer), right? src/java.base/share/classes/java/lang/Module.java line 115: > 113: > 114: // is this module a native module > 115: private volatile boolean enableNativeAccess = false; Can you drop "= false", it's not needed and I don't think we want a volatile-write here. Also the comment may date from a previous iteration as there isn't any concept of a "native module". src/java.base/share/classes/java/lang/System.java line 2346: > 2344: public boolean isEnableNativeAccess(Module m) { > 2345: return m.isEnableNativeAccess(); > 2346: } Can you move this up so they are with the other Module methods? src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java line 34: > 32: import java.util.Set; > 33: > 34: public final class IllegalNativeAccessChecker { Are you sure about the name of the this class? It doesn't do any checking and it's not concerned with "illegal native access" either, instead it just provides access to the names of modules that have been granted native access. src/java.base/share/classes/jdk/internal/module/IllegalNativeAccessChecker.java line 47: > 45: private static IllegalNativeAccessChecker checker; > 46: > 47: static Collection<String> enableNativeAccessModules() { I assume this can be changed to Iterable<String> as you don't want anything outside of this class changing the collection. src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 889: > 887: } else { > 888: // silently skip. > 889: // warnUnknownModule(ENABLE_NATIVE_ACCESS, name); Maybe for later but the other options do emit a warning when unknown module is specified. If the decoding of this command line option is moved to ModuleBootstrap then most of this class will go away and you will be able to use warnUnknownModule. src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 113: > 111: if > (!SharedSecrets.getJavaLangAccess().isEnableNativeAccess(module)) { > 112: String moduleName = module.isNamed()? module.getName() : > "UNNAMED"; > 113: throw new IllegalCallerException("Illegal native access from > module: " + moduleName); "UNNAMED" is a bit inconsistent with the other exception messages. Can you just use Module::toString here instead, meaning "Illegal native access from " + module ? ------------- PR: https://git.openjdk.java.net/jdk/pull/3699
