Fwd: Re: Review request: 8003562: Provide a command-line tool to find static dependenciesh
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/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.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
>
More information about the core-libs-dev
mailing list