The latest webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.03/
--alex
On 08/31/2018 10:49, Alex Menkov wrote:
Hi Jc,
On 08/31/2018 10:25, JC Beyler wrote:
Hi Alexey,
Apart from Serguei's comment above, I only saw two nits:
A) The shorthand
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html>
1) typo shortland -> shorthand
2) could we just have a method instead of the shorthand? ie:
instead of:
+ protected final String debuggeeClass; // shortland for
launchOptions.debuggeeClass
we have:
+ protected String debuggeeClass() { return
launchOptions.debuggeeClass; }
Somehow, even as a final, duplication of data seems weird to me. (On
the other hand, it seems you only use launchOptions.debuggeeClass in
two spots in the code and once you used the shorthand and once you
didn't. So perhaps no shorthand at all makes sense?)
Yes, agree - there is no sense to have the field.
B) Not testing the command that works?
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NotAField.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NotAField.java.udiff.html>
-> I'm surprised we are testing a command that should work and then a
command that should not but only are checking the failing command's
output. I know this is a direct translation of the test but it seems
weird to not be testing also the hashCode() call, no?
The original issue (https://bugs.openjdk.java.net/browse/JDK-4467887) is
about "more graceful message when user tries to evaluate a method such
as length() or toString() as a field".
So actually this "correct" command is not needed (I suppose the command
is executed just because test example in the bug contains it for
comparison)
I'll do a test run and then upload new webrev.
--alex
Thanks,
Jc
On Thu, Aug 30, 2018 at 11:13 PM serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com>> wrote:
Hi Alex,
It looks good in general but not sure I understand all the changes.
Could you, please, tell a little bit more about the refactoring in
the Jdb.java and JdbTest.java?
I understand that you moved some code from Jdb.java to JdbTest.java.
But it is hard to track all of these move details.
What was the main refactoring logic?
Some comments on this update:
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NullLocalVariable.java.frames.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NullLocalVariable.java.frames.html>
30 * @library /test/lib
31 * @compile -g JdbExprTest.java
32 * @run main/othervm JdbExprTest
Why the class 'JdbExprTest' is used above?
Should it be the 'NullLocalVariable' instead?
Thanks,
Serguei
On 8/30/18 16:27, Alex Menkov wrote:
Hi all,
Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8210243
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/>
The fix converts the following tests:
- test/jdk/com/sun/jdi/JdbArgTest.sh
- test/jdk/com/sun/jdi/JdbLockTest.sh
- test/jdk/com/sun/jdi/JdbMissStep.sh
- test/jdk/com/sun/jdi/JdbVarargsTest.sh
- test/jdk/com/sun/jdi/MixedSuspendTest.sh
- test/jdk/com/sun/jdi/NotAField.sh
- test/jdk/com/sun/jdi/NullLocalVariable.sh
JdbArgTest requires to run only jdb (without debuggee), so Jdb
class was decoupled - it now contains only jdb logic, debuggee
logic was moved to JdbTest.
--alex
--
Thanks,
Jc
