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.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
