On 09/14/2018 15:03, JC Beyler wrote:
Hi Alex,
I also saw two nits, in the same file as Chris was mentioning
(http://cr.openjdk.java.net/~amenkov/sh2java/step4/webrev.01/test/jdk/com/sun/jdi/RedefineTTYLineNumber.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step4/webrev.01/test/jdk/com/sun/jdi/RedefineTTYLineNumber.java.udiff.html>),
there is a comment:
// 11 before, 10 afterward
That is right below, we could remove it now, no?
Also inhttp://cr.openjdk.java.net/~amenkov/sh2java/step4/webrev.01/test/jdk/com/sun/jdi/StringConvertTest.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step4/webrev.01/test/jdk/com/sun/jdi/StringConvertTest.java.udiff.html>:
+ // An unreported bug: this isn't handled correctly. + // If we
uncomment this line, we will get an 'instance of...' line + // which
will cause the test to fail. + //
jdb.command(JdbCommand.print("(Object)(StringConvertTarg.x3)"));
Should we perhaps file that bug now?
The log from 2 print calls:
> print ((Object)StringConvertTarg.x3).toString()
[jdb] com.sun.tools.example.debug.expr.ParseException: No instance field
or method with the name toString in StringConvertTarg$JJ2[]
[jdb] ((Object)StringConvertTarg.x3).toString() = null
[jdb] main[1]
> print (Object)(StringConvertTarg.x3)
[jdb] (Object)(StringConvertTarg.x3) = instance of
StringConvertTarg$JJ2[2] (id=624)
[jdb] main[1]
To me it looks correct as StringConvertTarg.x3 is an array.
The test is for 4511950 and 4843082, there is a comment for the 2nd one:
---
This fix also reverts the behavior of the command
print array
to the 1.4.1 behavior which is incorrect, but is better than the behavior
caused by the fix for 4511950.
---
So I suppose fix for 4511950 introduced some other output for arrays
which was reverted later.
So I think the comment may be removed.
--alex
Apart from that, looks good to me!
Thanks!
Jc
On Fri, Sep 14, 2018 at 2:28 PM Chris Plummer <chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com>> wrote:
Hi Alex,
Just one issue I see. For RedefineTTYLineNumber.java, the original test
used to have this comment, which your removed:
52 // line number sensitive!!! Next line must be line 10.
It's not clear to me why this test was ever line number sensitive, and
whether you removed this sensitivity, or it just never existed. In any
case, you left in the following comment, which maybe should also be
removed:
47 System.out.println("in A, about to call B"); // 11
before, 10 afterward
Also, the println output from A() does not seem to match what the test
is doing. There is no call to B():
46 public void A() {
47 System.out.println("in A, about to call B"); // 11
before, 10 afterward
48 System.out.println("out from B");
49 }
Maybe that's some bit rot. My understanding of the point of the test is
while at the breakpoint at the start of A(), a redefine is done that
deletes a line above this point, and jdi needs to make the appropriate
adjustment of the current breakpoint line number. So calling B() does
not play a roll in this, but perhaps it did a one point but the call
was
removed.
Also, I don't see any indication of line number sensitivity here, but
once again, maybe this is a bit rot issue and at one point it was line
number sensitive.
thanks,
Chris
On 9/14/18 12:59 PM, Alex Menkov wrote:
> Hi all,
>
> please review fix for
> https://bugs.openjdk.java.net/browse/JDK-8210760
> webrev:
> http://cr.openjdk.java.net/~amenkov/sh2java/step4/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step4/webrev.01/>
>
> --alex
--
Thanks,
Jc
