Fwd: Re: Review request: 8003562: Provide a command-line tool to find static dependenciesh
Ulf Zibis
Ulf.Zibis at CoSoCo.de
Thu Dec 20 13:06:38 PST 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 <level> --verbose-level=<level> Print package-level or class-level dependencies
Valid levels are: "package" and "class"
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20121220/5364ddf2/attachment.html
More information about the compiler-dev
mailing list