On Jun 13, 2013, at 6:47 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> Alan, Paul, > > Thanks for the review. Here is the revised webrev: > http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8015912/webrev.01 > Looks OK. > See my comments inlined below. > > On 6/12/2013 3:48 AM, Paul Sandoz wrote: >> Hi Mandy, >> >> JdepsTask: >> >> Given that the dot graph output is likely to be just as human readable as >> your original format it is very tempting just to support that format. In >> fact it is likely to be more human readable since the output is of a known >> syntax. So IMHO we don't need a format option and dot graph output is >> sufficient. > > The package names and classnames in the dot graph output are double-quote > strings or the '.' character has to be escaped. I think it's more of a > noise as the default output. I am not really convinced the quoting is noisy, i think having two formats is noise :-) It is still very readable: digraph "a.jar" { "x.y.z.Foo" -> "a.b.c.Bar"; .... } Also, did you also consider using subgraph for multiple archives? > >> >> 636 log.format("digraph G {%n"); >> >> Perhaps a better name can be chosen for the title of the summary graph? > > Oh yes - I change it to "jdeps summary". Is it any better? >> Yes. >> You don't need to pass in the log PrintWriter to the OutputWriter >> implementations if they are non-static i.e. sometimes you use "out" and >> sometimes you use "log". > > I agree. Fixed. >> >> PlatformClassPath: >> >> 53 static boolean contains(Archive archive) { >> 54 return javaHomeArchives.contains(archive) && >> !archive.getFileName().equals("jfxrt.jar"); >> 55 } >> >> Just curious: why is the the check for "jfxrt.jar" required? > > If jfxrt.jar is bundled with JRE, it's in the jre/lib/ext directory but it's > not part of the JRE and not in any profile. Thus it should be filtered out. > An outlier! Can other stuff get placed in that dir? Paul.
