[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-nashorn-dev
Subject:    Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API
From:       Hannes Wallnoefer <hannes.wallnoefer () oracle ! com>
Date:       2016-05-31 7:32:35
Message-ID: 574D3E13.4020601 () oracle ! com
[Download RAW message or body]

+1 for the incremental changes in the last iteration.

Hannes

Am 2016-05-31 um 09:17 schrieb Sundararajan Athijegannathan:
> 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.athijegannathan@oracle.com
>>>>> <mailto:sundararajan.athijegannathan@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
>>>>
>>>>

[prev in list] [next in list] [prev in thread] [next in thread]