Hi Serguei,
Thanks very much for the review and I have made all the changes
incorporating your feedback.
New Webrev at :http://cr.openjdk.java.net/~psomashe/8196324/webrev.01/
<http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.01/>
thanks,
Paru.
On 2/13/18, 2:02 PM, serguei.spit...@oracle.com wrote:
Hi Paru,
It looks good in general but I'd like to ask for some cleanup.
I tell just about the first test (FilterMatch) but the other one needs
the same.
132 // This gets called if all filters match.
133 public void stepCompleted(StepEvent event) {
134 listenCalled = true;
135 System.out.println("listen: line#=" +
event.location().lineNumber()
136 + " event=" + event);
137 // disable the step and then run to completion
138 StepRequest str= (StepRequest)event.request();
139 str.disable();
140 eventSet.resume();
141 }
I'd suggest to replace "listen:" above with "Agent: StepEvent:" to make
it more consistent with similar tracing in the breakpointReached() below.
143 public void breakpointReached(BreakpointEvent event) {
144 ThreadReference thread = ((BreakpointEvent) event).thread();
145 String locStr = "" + ((BreakpointEvent) event).location();
146 System.out.println("Agent: BreakpointEvent #" +
147 " at " + locStr + " in thread: " + thread);
148 // The bkpt was hit; disable it.
149 request.disable();
150 }
The casts to BreakpointEvent at lines 144, 145 are not needed
as the "event" is already of this type.
Unneeded sign '#' at the line 146.
Also, I'm suggesting to disable the "request" the same way
as it is done in the stepCompleted():
BreakpointRequest bpr= (BreakpointRequest)event.request();
bpr.disable();
It will help to make the "request" local in the runTests().
Unneeded empty lines: 52, 54:
51 EventSet eventSet = null;
52
53 static boolean listenCalled;
54
55 BreakpointRequest request;
The listenCalled needs to be renamed to stepCompleted.
There is no big need for it to be static as it is similar to the eventSet.
It is better to initialize it with false.
Then this line can be removed:
114 listenCalled = false;
I'd suggest to rename the variables "request" and "request1"
to "bpRequest" and "stepRequest" and make the 'bpRequest' to be local
in the runTests() as it is the only place where it has to be used.
Thanks,
Serguei
On 2/12/18 11:25, Paru Somashekar wrote:
Hi,
Please review fix for JDK-8196324
The includes the following changes:
* Use startToMain method of TestScaffold that automatically calls
connect() and waitForVMStart()
* Since TestScaffold extends TargetAdapter, it now overrides event
callbacks directly in the test class rather than implement an
anonymous inner class.
* Breakpoint handling is done in breakpointReached callback instead
of waitForRequestedEvent and subsequently removing it.
Bug : https://bugs.openjdk.java.net/browse/JDK-8196324
Review : http://cr.openjdk.java.net/~psomashe/8196324/webrev/
<http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/>
<http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/>
thanks,
Paru.
