Looks good.
Chris
On 9/4/18 11:01 AM, Alex Menkov wrote:
Hi Chris,
fixed.
Updated webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.04/
On 08/31/2018 15:53, Chris Plummer wrote:
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.
Because the classes have similar functionality.
Initially it was planned to use vmTestBase classes to shell->java test
conversion, but I was told by SQE that vmTestBase tests have too many
issues and need to be reworked (moving them to appropriate locations).
--alex
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
