Fwd: Re: Review request: 8003562: Provide a command-line tool to find static dependenciesh

Mandy Chung mandy.chung at oracle.com
Thu Dec 20 21:46:08 UTC 2012


On 12/20/12 1:06 PM, Ulf Zibis wrote:
> Hi Mandy,
>
> I had much to do, guessed, all would be all-right.
>
> I first had problems to find my proposed changes from 2012-12-15 in 
> your code.
> Now I found it. You did it even better
>
> Just an additional nit in JDepsTask:
>  511                 if (o.hasArg) {
>  512                     String arg = null;
>  513                     if (name.startsWith("--") && 
> name.indexOf('=') > 0) {
>  514                         arg = name.substring(name.indexOf('=')+1, 
> name.length());
>  515                     } else {
>  516                         arg = rest.hasNext() ? rest.next() : "-";
>  517                     }
>  518                     if (arg.startsWith("-")) {
>  519                         throw new BadArgs("err.missing.arg", 
> name).showUsage(true);
>  520                     } else {
>  521                         o.process(this, name, arg);
>  522                     }
>  523                 } else {
>  524                     o.process(this, name, null);
>  525                 }
>
> Could just be:
>  511                 String param = null;
>  512                 if (o.hasArg) {
>  513                     if (name.startsWith("--") && 
> name.indexOf('=') > 0) {
>  514                         param = 
> name.substring(name.indexOf('=')+1, name.length());
>  515                     } else if (!rest.hasNext() || (param = 
> rest.next()).startsWith("-")) {
>  516                         throw new BadArgs("err.missing.param", 
> name).showUsage(true);
>  517                     }
>  518                 }
>  519                 o.process(this, name, param);
> This additionally would allow negative values for the outlined option.
> (to avoid confusion with the upper loop, better rename arg-->param)

I can fix this.  I tend not to inline the assignment in a long statement 
with multiple-evaluation for readability.  For this case, I like your 
suggestion to be concise while readable.
>
> You do not like for loops? :

I like for-loop :) What you suggested last time and below has a bug

>  492         boolean acceptOption = true;
>  493         while (iter.hasNext()) {
>  494             String arg = iter.next();
>  495             if (arg.startsWith("-") && acceptOption) {
> Could be:
>  492         boolean acceptOption = true;
>  493         for (String arg; iter.hasNext(); arg = iter.next()) {

arg is initialized after the first iteration.  I could do
    for (; iter.hasNext() && ((arg = iter.next()) != null);) {
       ...
    }

but I don't see how it's much prettier than the while-loop above (2-line 
into 1-line).  I considered to use foreach and move away from Iterator.

>  494             if (acceptOption && arg.startsWith("-")) { // swap 
> order for better performance
> Additionally while loops tend to JIT-compile bad, see:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6932855
>

OK.
> *Not* a nit:
> You do not handle a missing option parameter for the outlined form. 

do you have an example what it doesn't handle?  If it's missing, the 
first given file will be used as the option parameter.  It validates the 
option parameter for the case we can.  There are cases we can't (e.g. 
classpath).

> So I think, you better handle existence and validity (e.g. wrong 
> leading '-') or parameters by process(this, name, param) and just code:
>  513                 o.process(this, name, (name.startsWith("--") && 
> name.indexOf('=') > 0)
>  514                         ? name.substring(name.indexOf('=')+1, 
> name.length());
>  515                         : o.hasArg && rest.hasNext() ? 
> rest.next() : null);
> This additionally would allow negative values for both option forms.
> As the algorithm now is very short, it could be easily inlined in the 
> calling method
>

I may be missing the problem you try to point out.  And this is harder 
to read for me :)
>
> About the options an additional thought:
>   -v         --verbose                 Print additional information
>   -V <level> --verbose-level=<level>   Print package-level or 
> class-level dependencies
>                                        Valid levels are: "package" and 
> "class"

I somewhat agree with this but I'd like to get this out for people to 
try and get feedback.   We can refine the options after the first push.  
I expect the tool will evolve and the options may be revised too.  For 
example there are some additional information we can enhance the tool to 
print out (e.g. there is a suggestion to include the method/field 
reference count from one class to another that will be a useful hint to 
indicate how interconnected a class depends on the other).

Mandy

> 1. I think, --verbose-level=<level> is too verbose, why not just 
> --verbose=<level> or maybe alternatively:
>   -l <level> --level=<level>   Print package-level or class-level 
> dependencies.
>   -D <level> --dependency-level=<level>   Print package-level or 
> class-level dependencies.
> 2. It's not clear if -v/--verbose without parameter means one of the 
> others with which default.
> 3. I more would like only 1 verbose option, something like:
>   -v <level> --verbose=<level>   Print additional information; optional
>                                        valid level dependencies: 
> "package" and "class"
>
> 4. Couldn't you use lower-case -r for --recursive ?
>
> -Ulf
>
>
> Am 20.12.2012 17:07, schrieb Mandy Chung:
>> Ulf,
>>
>> Thanks for all the feedback. Just to double check - are you ok with 
>> the new webrev?
>>
>> Mandy
>>
>> -------- Original Message --------
>> Subject: 	Re: Review request: 8003562: Provide a command-line tool to 
>> find static dependencies
>> Date: 	Tue, 18 Dec 2012 15:12:39 -0800
>> From: 	Mandy Chung <mandy.chung at oracle.com>
>> To: 	Ulf Zibis <Ulf.Zibis at CoSoCo.de>, Alan Bateman 
>> <alan.bateman at oracle.com>
>> CC: 	core-libs-dev at openjdk.java.net, compiler-dev at openjdk.java.net
>>
>>
>>
>> Alan, Ulf,
>>
>> I updated the jdeps CLI per the discussion we had.
>>
>> $ jdeps --help
>> Usage: jdeps<options>  <classes...>
>> where<classes>  can be a pathname to a .class file, a directory, a JAR file,
>> or a fully-qualified classname or wildcard "*".  Possible options include:
>>    -s         --summary                 Print dependency summary only
>>    -v         --verbose                 Print additional information
>>    -V<level>  --verbose-level=<level>    Print package-level or
>> class-level dependencies
>>                                         Valid levels are: "package" and
>> "class"
>>    -c<path>   --classpath=<path>         Specify where to find class files
>>    -p<pkg name>  --package=<pkg name>    Restrict analysis to classes in
>> this package
>>                                         (may be given multiple times)
>>    -e<regex>  --regex=<regex>            Restrict analysis to packages
>> matching pattern
>>                                         (-p and -e are exclusive)
>>    -P         --profile                 Show profile or the file
>> containing a package
>>    -R         --recursive               Recursively traverse all
>> dependencies
>>               --version                 Version information
>>
>> Updated webrev:
>>     http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.06/
>>
>> jdeps only support GNU-style options.  I added java.util.function
>> and com.sun.source.doctree in the jdk.properties.  I'll replace
>> it to use the proper javac API to work with profiles next.  I
>> caught a typo 'com.sunsource.doctree' (missing dot) in NON_CORE_PKGS.gmk
>> and I fix that in this patch.
>>
>> We can enhance the tool after the initial push.  I'd like to get it
>> in jdk8 soon so that developers can try it out and give feedback.
>>
>> Thanks
>> Mandy
>




More information about the core-libs-dev mailing list