Hi Serguei,
Updated webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.03/
--alex
On 09/11/2018 22:15, [email protected] wrote:
Hi Alex,
It looks good.
Just some minor comments.
http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html
+ public LaunchOptions sourceFilename(String name) {
+ sourceFilename = name;
+ return this;
+ }
A suggestion is to rename this method to setSourceFilename.
http://cr.openjdk.java.net/~amenkov/sh2java/redefineClasses1/webrev.02/test/jdk/com/sun/jdi/lib/jdb/ClassTransformer.java.html
53 public ClassTransformer fileName(String fileName) {
54 this.fileName = fileName;
55 return this;
56 }
A suggestion is to rename this method to setFileName;
It would be better for the comment at lines 107-135 to use the form:
/*
* ...
*/
Thanks,
Serguei
On 9/11/18 12:28, Alex Menkov wrote:
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
