Hi Alex,
Overall it looks good.
159 // jdb prompt when debuggee is not started nor suspended after
breakpoint
How about "and is not suspended". And I'm not so sure the "after
breakpoint" is needed. Jdb always suspends after a breakpoint.
Eventually the user does a "cont", and after that I think you always see
the simple prompt "> " because after the "cont" the debuggee is not
suspended anymore.
I'm just now realizing that there is a lot of replication of prompt
related code in the vmTestBase version of Jdb.java. Maybe that's
something we can address in the future.
thanks,
Chris
On 8/31/18 11:46 AM, Alex Menkov wrote:
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
