8181087: Module system implementation refresh (6/2017 update)

[Peter Levart]

Re-reading my post in the morning...

On 06/14/2017 11:44 PM, Peter Levart wrote:
> In j.l.Module:
>
> There are two places in the same method that contain exactly the same 
> fragment of code:
>
>  566                 if (targets.contains(EVERYONE_MODULE) || 
> targets.contains(other))
>  567                     return true;
>  568                 if (other != EVERYONE_MODULE
>  569                     && !other.isNamed() && 
> targets.contains(ALL_UNNAMED_MODULE))
>  570                     return true;
>
> Perhaps this could be factored out into separate private method which 
> could also be made a little more optimal (when other == 
> EVERYONE_MODULE and targets does not contain it, it is looked-up twice 
> currently). For example:
>
> private static boolean isIncludedIn(Module module, Set<Module> targets) {
>     return
>         targets != null && (
>             targets.contains(EVERYONE_MODULE) ||
>             !module.isNamed() && targets.contains(ALL_UNNAMED_MODULE) 
> || // ALL_UNNAMED_MODULE.isNamed() == false
>             module != EVERYONE_MODULE && module != ALL_UNNAMED_MODULE 
> && targets.contains(module)
>          );
> }

...this last method is not entirely correct. if called as 
isIncluded(EVERYONE_MODULE, targets) and targets does not contain 
EVERYONE_MODULE but it contains ALL_UNNAMED_MODULE, the method returns 
true, because EVERYONE_MODULE.isNamed() returns false, which is not 
correct I think. The correct logic would be this:

private static boolean isIncludedIn(Module module, Set<Module> targets) {
     return
         targets != null && (
             targets.contains(EVERYONE_MODULE) ||
             module != EVERYONE_MODULE && !module.isNamed() && 
targets.contains(ALL_UNNAMED_MODULE) || // ALL_UNNAMED_MODULE.isNamed() 
== false
             module != EVERYONE_MODULE && module != ALL_UNNAMED_MODULE 
&& targets.contains(module)
          );
}



Regards, Peter