RFR: jmod remove

[Chris Hegarty]

On 10/05/12 22:59, Mandy Chung wrote:
> Chris,
>
> Looks good in general. Some minor comments below:

Thanks Mandy, I made all your suggested cleanup changes.

 > L1546-1548: Another way to perform this check would be:
 > for (ModuleId mid : mids) {
 > if (cf.findContextForModuleName(mid) != null)
 > throw new ....
 > }

In its current form the above won't work. findContextForModuleName takes 
a String module name, what we want to determine is if a particular 
module id is being used by the configuration. The Configuration itself ( 
and it's maps ) doesn't directly know what module id is being used, you 
need to dig a little deeper into the actual contexts.

I guess this is somewhat related to whether we want to support alias 
and/or non-default view names. Let's leave it as is for now. We can 
revisit it later if necessary. I added a comment.

Thanks,
-Chris.

> On 5/9/2012 7:31 AM, Chris Hegarty wrote:
>> http://cr.openjdk.java.net/~chegar/jigsaw/jmodrm.06/webrev/
>
> Files.java
> You can also remove L39.
> L147: Could you just add exc to the "excs" list rather
> than wrapping it with a new IOException?
>
> Librarian.java
> L317: The "excs" variable is not needed any more.
> L327: Should warnings normally go to System.err instead of
> System.out?
>
> SimpleLibrary.java
> L1505: should the remove method accept a declaring ModuleId
> as a valid argument? I am wondering whether it should allow
> to remove a module if an alias name or non-default view name
> is given. Perhaps add a comment and leave it as an open issue
> to be addressed later.
>
> L1546-1548: Another way to perform this check would be:
> for (ModuleId mid : mids) {
> if (cf.findContextForModuleName(mid) != null)
> throw new ....
> }
> Would that be more explicit?
>
> ModuleFile.java
> L525: should be aligned 2-space to the left
>
> Mandy
> P.S. I hope to send you my comments this morning so that you
> can rev it once. I was almost done before going to 11am and
> got tied up until now. Sorry for causing you to generate
> another round of webrev.
>