Review Request: JDK-8171418: Remove jdeps internal --include-system-modules option

[Daniel Fuchs]

On 19/12/16 18:06, Mandy Chung wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171418/webrev.01

Looks good to me.
A related question is whether the tokens ALL-SYSTEM, ALL-DEFAULT,
ALL-MODULE-PATH, should appear in the jdeps help message?

best regards,

-- daniel

>
>> On Dec 19, 2016, at 9:54 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi Mandy,
>>
>> DepsAnalyzer.java:
>>
>> 232         Collection<Module> modules = configuration.getModules().values();
>>
>> I don't see where modules is used in this method.
>> Should this line be removed?
>>
>
> Removed. Lance also caught that.
>
>>
>> 163         if (filter.requiresFilter().isEmpty()) {
>> 164             return archives.stream()
>> 165                 .filter(this::include)
>> ...
>> 168         } else {
>> 169             // use the archives that have dependences and not specified in --require
>> 170             return archives.stream()
>> 171                 .filter(source -> !filter.requiresFilter().contains(source.getName()))
>> 172                 .filter(this::include)
>> ...
>>
>> and later:
>>
>> 266     public boolean include(Archive source) {
>> 267         Module module = source.getModule();
>> 268         // skip system module by default
>> 269         return  !module.isSystem()
>> 270                     || configuration.rootModules().contains(source)
>> 271                     || filter.requiresFilter().contains(module.name());
>> 272     }
>>
>>
>> If I look at how JdepsFilter::includes was implemented, and given that
>> filter.requiresFilter().contains(module.name()) will always be false
>> if we come from line 165, or from line 172 then I would suggest to
>> remove line 271: this would ensure that the call at line 95 does
>> the same than before.
>>
>
> That’s a good suggestion.
>
> Mandy
>
>>
>> On 19/12/16 05:42, Mandy Chung wrote:
>>> Webrev at:
>>>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171418/webrev.00
>>>
>>> --include-system-modules option was a temporary option added in an early stage of developement.  Time to remove it as a clean up.
>>>
>>> Mandy
>>>
>>
>