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

Ulf Zibis Ulf.Zibis at CoSoCo.de
Thu Dec 20 21:06:38 UTC 2012

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)

You do not like for loops? : 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()) { 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

Not a nit: You do not handle a missing option parameter for the outlined form. 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

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"

  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