Review request for jdeps option rename to Java-style and enhancements to output dot file format

Mandy Chung mandy.chung at oracle.com
Tue Oct 15 03:38:11 UTC 2013


Alan,

Thanks for the review.   Here is the updated webrev per your feedback:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8015912/webrev.01/

I have also fixed 
jdk/test/sun/reflect/CallerSensitive/CallerSensitiveFinder.java due to 
the change of ClassFileReader.newInstance method.

More comments inlined below.

On 10/13/2013 1:31 PM, Alan Bateman wrote:
> Command line options are somewhat subjective, I think what you have is 
> reasonable. I thought the GNU style that were in the original version 
> were okay too (just takes getting used to) but keeping them consistent 
> with similarly named options in the other tools is important tool. One 
> change that I didn't quite understand is dropping -R for recursive, 
> that one seemed intuitive (to me).
>

Initially I considered that -recursive is a less frequently used option 
as typically we would be interested in the immediate dependencies.  I 
also think that -R is an intuitive short option to -recursive and there 
shouldn't be an issue when it's upgraded to gnu-style in the future and 
so I add it back.

> Anyway, on the changes then I went through the webrev and I didn't see 
> anything obviously wrong. A few comments:
>
> - In PlatformClassPath then alt-rt is special-cased and I think that 
> can be removed now.
>

That's right (thanks for reminding it).  I have removed it.

> - Also in PlatformClassPath is special handling of jfxrt.jar (if it is 
> present) isn't clear. I recall this came up before and maybe it just 
> needs a more detailed comment to explain why it is needed.
>

PlatformClassPath.contains method seems to be a bit confusing.  I added 
a new JDKArchive class that will be instantiated for JDK jar file

> - In Analyzer then I think I missed why the results are a 
> LinkedHashMap, wouldn't HashMap do?
>

It doesn't need to be ordered and I have changed it to HashMap. That was 
a left-over change I made in an earlier version of this fix.   JdepsTask 
already maintains an ordered list of archives (sourceLocations).

> - A minor comment in JdepsTask but I assume the OutputWriter interface 
> can be private
>

OutputWriter was in an earlier version but was removed.  It's not in 
webrev.00 and perhaps you were looking at an earlier version?

Mandy



More information about the core-libs-dev mailing list