Review request on the optional modules support

[Mandy Chung]

On 8/8/11 8:01 AM, Alan Bateman wrote:
> Mandy Chung wrote:
>> An update on the optional module support [1].
>>
>> Webrev:
>>    
>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>>
>> The note has been updated to include the open questions raised in
>> the discussion.
>>    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
> I went through the webrev.
>
> It mostly looks okay to me but I suspect that developers might 
> initially grapple with isModulePresent and requireModulePresent as 
> instance methods (in time I guess the javadoc will be fleshed out and 
> concepts such as context can be referenced).
>

I think ModuleClassLoader might actually be a more appropriate class 
defining these methods.  The downside of it is that it requires to cast 
the class loader returned from Class.getClassLoader() to ModuleClassLoader.

> I see you've changed a few of the SecurityManager methods to throw 
> ModuleNotPresentException but I'm not sure that is really needed. The 
> alternative is to just have the permission checks fail, as they do 
> now, when the desktop module is not present.
>

There doesn't seem to have any reason for an application that doesn't 
require desktop module to call these methods.  So the question is 
whether we want to disallow the use of these methods if desktop module 
is not present:
    checkAwtEventQueueAccess()
    checkTopLevelWindow()
    checkSystemClipboardAccess()

It's more of a cleanup on the API.

> The updates to java.lang.reflect.Module remind me to ask whether this 
> is the right package for this class (java.lang, java.lang.reflect or 
> java.lang.module). I realize there is history here (should Class and 
> Package be in java.lang.reflect, etc.) but it does seem inconsistent now.
>

I too wonder about the inconsistency.  I'm making a note of it.

> One of the questions posed is whether the compiler can use the 
> RequireOptionalModule annotations but I would have thought that if 
> there are any references to types in optional modules then it will 
> require that they be present at compile time. Maybe this question is 
> only for the case that reflection is used?
>

Roger posted this question in our previous discussion.  At compile time, 
the optional module must be present if there is any reference to types 
in the optional module.   If the optional method is not carefully 
written, at class loading time, the verifier might load types in 
optional modules for verification regardless of whether the optional 
module is called or not.  The question really comes to whether the 
compiler can detect such potential verification error during 
compilation.  I talked with Jon Gibbons another day.  The compiler can't 
detect that.  Jon and Alex can chime in and explain further.

> Another question is about handling different versions but that strikes 
> me as a bit fragile and could potentially get out of sync with the 
> version constraints in the module-info too.
>
> Otherwise I think the changes are okay and I note that 
> RequireOptionalModule will need to have to classes element removed 
> once we don't require the class analyzer in the build.

I may just take it out now since the class analyzer is using the 
depconfig file.

Thanks
Mandy