Re: Review for 8006225: tools/jdeps/Basic.java fails with AssertionError

2013-02-14 Thread Alan Bateman

On 14/02/2013 01:52, Mandy Chung wrote:

This fixes 8006225: tools/jdeps/Basic.java fails with AssertionError.

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8006225/webrev.00/

jdeps maintains a static list of analyzed classes, each of which is 
expected to be from one archive.  test/tools/jdeps/Basic.java calls 
com.sun.tools.jdeps.Main multiple times with different classpath and 
thus the same class came from different path in a different invocation 
of jdeps.  Fix jdeps to keep the data in instance variables.  I 
refactored the analysis from JdepsTask into a new Analyzer class to 
ease further enhancement.
This looks okay to me, I guess it's only going to be a test that runs it 
more than once in the same VM that would have noticed this, someone 
using the tool from the command-line would not.


-Alan


Re: Review for 8006225: tools/jdeps/Basic.java fails with AssertionError

2013-02-14 Thread Mandy Chung

On 2/14/13 7:43 AM, Alan Bateman wrote:

On 14/02/2013 01:52, Mandy Chung wrote:

This fixes 8006225: tools/jdeps/Basic.java fails with AssertionError.

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8006225/webrev.00/

jdeps maintains a static list of analyzed classes, each of which is 
expected to be from one archive.  test/tools/jdeps/Basic.java calls 
com.sun.tools.jdeps.Main multiple times with different classpath and 
thus the same class came from different path in a different 
invocation of jdeps.  Fix jdeps to keep the data in instance 
variables.  I refactored the analysis from JdepsTask into a new 
Analyzer class to ease further enhancement.
This looks okay to me, I guess it's only going to be a test that runs 
it more than once in the same VM that would have noticed this, someone 
using the tool from the command-line would not.



Right.  The tool from command-line doesn't run into that issue.

Thanks for the review.
Mandy


Review for 8006225: tools/jdeps/Basic.java fails with AssertionError

2013-02-13 Thread Mandy Chung

This fixes 8006225: tools/jdeps/Basic.java fails with AssertionError.

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8006225/webrev.00/

jdeps maintains a static list of analyzed classes, each of which is 
expected to be from one archive.  test/tools/jdeps/Basic.java calls 
com.sun.tools.jdeps.Main multiple times with different classpath and 
thus the same class came from different path in a different invocation 
of jdeps.  Fix jdeps to keep the data in instance variables.  I 
refactored the analysis from JdepsTask into a new Analyzer class to ease 
further enhancement.


Thanks
Mandy