Hi Jc,
Thank you for review.
Updated webrev (fixed both issue noted):
http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/
--alex
On 09/10/2018 21:00, JC Beyler wrote:
Hi Alex,
I was looking at this and I have a few nits and questions:
- I saw two spots in the patch that had weird spaces:
test/jdk/com/sun/jdi/RedefineG.java
+ super( DEBUGGEE_CLASS,
+ "RedefineG.java");
- Then test/jdk/com/sun/jdi/RedefineImplementor.java
+ super( RedefineImplementorTarg.class.getName(),
+ "RedefineImplementor.java");
- My only question is should we not be worried or care if both
test/jdk/com/sun/jdi/lib/jdb/JdbTest.java and
test/jdk/com/sun/jdi/lib/jdb/ClassTransformer.java have now ways that could
provoke NPEs? Should the code not try to throw an exception and say: careful,
you called this but didn't call that first? It would make misuse of the class
easier to debug when writing new tests.
(Example being: calling redefineClass but not using the right constructor)
I know it is for testing only but if we start writing new tests, it could be
useful to be a bit defensive?
Apart from that, looked good to me.
Jc
On Mon, Sep 10, 2018 at 4:40 PM Alex Menkov <[email protected]
<mailto:[email protected]>> wrote:
Hi,
Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8210560
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/redefineClasses1/webrev.01/>
New class (ClassTransformer) was developed to implement simple class
transformer for class redefinition (the same functionality as
implemented by ShellScaffold.sh).
Both setBreakpoint and redefineClass require source file name, so
JdbTest class was updated to accept it as ctor argument and use by
corresponding methods.
--alex
--
Thanks,
Jc
