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.ch...@oracle.com>
To: Ulf Zibis <ulf.zi...@cosoco.de>, Alan Bateman <alan.bate...@oracle.com>
CC: core-libs-dev@openjdk.java.net, compiler-...@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
