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
