Alex,
Thank you for the update!
It looks good.
There is another instance with incorrect spacing:
121 totRes=doPopFrame(false, Thread.currentThread());
No need in new webrev if you fix the above.
Thanks,
Serguei
On 12/18/18 6:01 PM, Alex Menkov wrote:
Hi Serguei,
Thank you for the review.
Updated webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.02/
On 12/18/2018 16:49, [email protected] wrote:
Hi Alex,
A couple of minor comments.
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe002.java.frames.html
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe004.java.frames.html
While you are at these files, could you, fix several originally
ugly indented comments?
Done.
Could you, also, fix the incorrect spacing around '=' in the
popframe004.java:
95 retValue=doPopFrame(true, popFrameClsThr);
Done.
Also fixed comment in popframe004/TestDescription.java (to be in sync
with comment change in popframe004.java)
Could you give an idea about the motivation to remove the
following fragment ?:
This is "test case 2" in popframe004 which I mentioned in the review
request.
The code is never executed (if it is, this means the test has already
failed) and I don't have an idea what other case can be tested here.
--alex
167 if (popframe004.popFdone) { // popping has been done
168 if (DEBUG_MODE)
169 out.println("popFrameCls (" + this +
170 "): enter activeMethod() after popping");
171 // test case #2: popping from the current thread
172 if (!popframe004.popF2done) {
173 popframe004.popF2done = true;
174 if (DEBUG_MODE) {
175 out.println("popFrameCls (" + this +
176 "): going to pop a frame from the current thread...");
177 retVal = doPopFrame(3, popFrameClsThr);
178 } else
179 retVal = doPopFrame(2, popFrameClsThr);
180 if (retVal != PASSED)
181 popframe004.totRes = FAILED;
182 }
183 if (DEBUG_MODE)
184 out.println("popFrameCls (" + this +
185 "): leaving activeMethod()...");
186 return;
187 }
Otherwise, it looks good.
Thanks,
Serguei
On 12/18/18 1:37 PM, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8215425
webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/
The fix
- turns on detailed output for the tests and cleaned related stuff;
- for popframe002 deletes output from run() method as it caused
unexpected MethodExit event;
- removes "test case 2" in popframe004 test as it's never executed.
--alex
