Sorry, I have updated nashorn webrev again: http://cr.openjdk.java.net/~sundar/8158131/nashorn/webrev.02/
Incremental changes: * Missed "nashornModule" name in JavaAdapterClassLoader.java. Fixed it to use all caps name * AccessController.doPrivileged call in Context.createModuleTrusted. Earlier I had used an acc. context with "getClassLoader" and "createClassLoader" permissions. But, Layer.defineModules call requires just "getClassLoader" permission. Reduced acc. context accordingly. -Sundar On 5/30/2016 4:13 PM, Sundararajan Athijegannathan wrote: > Updated nashorn webrev: > http://cr.openjdk.java.net/~sundar/8158131/nashorn/webrev.01/ > > * Added @link in ModuleGraphManipulator.java's javadoc > > * Using Context.class.getModule() in all places where nashorn module is > needed > > * Using all caps names for static final variables - nashornModule, > javaBaseModule, myModule in generated > > -Sundar > > > On 5/30/2016 2:50 PM, Sundararajan Athijegannathan wrote: >> Hi, >> >> Inline replies below. >> >> >> On 5/30/2016 2:16 PM, Michael Haupt wrote: >>> Hi Sundar, >>> >>> lower-case thumbs up for both the jdk and nashorn changes, with remarks: >>> >>> == ModuleGraphManipulator.java == >>> >>> * please add a @see or @link to the place where the bytes are read and >>> code is dynamically generated, or used, and vice versa. >>> >> Will do. >> >>> == JavaAdapterBytecodeGenerator.java == >>> >>> + private static final Module nashornModule = >>> JavaAdapterBytecodeGenerator.class.getModule(); >>> >>> This borders on the emerging discipline of Jigsaw idioms and patterns >>> ... assuming this class moves, at some point in the future, to another >>> (internal?) module. This will itch. How about having one central >>> authority saying "I represent the Nashorn module", rather than >>> implicitly assuming the class at hand will be in that module forever? >>> One line below, Object.class.getModule() looks like a waterproof >>> representative for the java.base module. >> hmm.. if this class moves out of nashorn , many things will break >> anyway! i.e., this has to be in nashorn (because of security assumptions >> for eg). I could use Context class perhaps.. >> >>> + private static byte[] MODULES_READ_ADDER_BYRES; >>> >>> If it cannot be final, how about @Stable? It would help, if not in >>> terms of performance, so at least to document that this is an array >>> that will be populated *once* and never change thereafter. >>> >> hmm.. @Stable would bring another internal API dependency to nashorn. >> Would like to avoid another internal API dependency to nashorn. >> >>> + // private static final Module myModule; >>> + { >>> + FieldVisitor fv = cw.visitField(ACC_PRIVATE | ACC_FINAL | >>> ACC_STATIC, >>> + "myModule", "Ljava/lang/reflect/Module;", null, null); >>> + fv.visitEnd(); >>> + } >>> >>> Suggestion: adhere to Java coding style even in generated code and >>> spell this as MY_MODULE. >> Will fix that. >> >> Thanks, >> -Sundar >> >>> Best, >>> >>> Michael >>> >>>> Am 30.05.2016 um 10:24 schrieb Sundararajan Athijegannathan >>>> <sundararajan.athijegannat...@oracle.com >>>> <mailto:sundararajan.athijegannat...@oracle.com>>: >>>> >>>> Please review http://cr.openjdk.java.net/~sundar/8158131/ >>>> <http://cr.openjdk.java.net/%7Esundar/8158131/> for >>>> https://bugs.openjdk.java.net/browse/JDK-8158131 >>>> >>>> This code cleanup is to avoid Nashorn's use of JDK internal class >>>> jdk.internal.module.Modules. >>>> >>>> With this change, nashorn uses java.lang.reflect.Layer public API to >>>> create single Module layers as needed. And nashorn generates/loads code >>>> into appropriate modules (which calls java.lang.reflect.Module.addReads, >>>> .addExports public APIs) to add module export/read edges as needed. >>>> >>>> -Sundar >>>> >>> -- >>> >>> Oracle <http://www.oracle.com/> >>> Dr. Michael Haupt | Principal Member of Technical Staff >>> Phone: +49 331 200 7277 | Fax: +49 331 200 7561 >>> Oracle Java Platform Group | LangTools Team | Nashorn >>> Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 >>> Potsdam, Germany >>> >>> ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, >>> D-80992 München >>> Registergericht: Amtsgericht München, HRA 95603 >>> >>> Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering >>> 163/167, 3543 AS Utrecht, Niederlande >>> Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 >>> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher >>> Green Oracle <http://www.oracle.com/commitment> Oracle is committed >>> to developing practices and products that help protect the environment >>> >>>
