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

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/viewbug.do?bugid=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 --verbose-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= is too verbose, why not just --verbose= or maybe alternatively: -l --level= Print package-level or class-level dependencies. -D --dependency-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 --verbose= 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 <classes...> where 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 --verbose-level= Print package-level or class-level dependencies Valid levels are: "package" and "class" -c --classpath= Specify where to find class files -p --package= Restrict analysis to classes in this package (may be given multiple times) -e --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 NONCOREPKGS.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