Thanks Serguei, made the changes you have suggested,
and the latest review is at:
http://cr.openjdk.java.net/~psomashe/8196324/webrev.02/
<http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.02/>
thanks,
Paru.
On 2/14/18, 2:30 PM, serguei.spit...@oracle.com wrote:
Hi Paru,
Thank you for the update!
Could you, please, rearrange a couple of more places in both files?
66 BreakpointEvent bpe = startToMain("HelloWorld");
67 ReferenceType referenceType =
(ClassType)bpe.location().declaringType();
68
69 mainThread = bpe.thread();
70 // VM has started, but hasn't started running the test program
yet.
71 EventRequestManager requestManager = vm().eventRequestManager();
72
73 Location loc = findLocation(referenceType, 3);
74
75 BreakpointRequest bpRequest =
requestManager.createBreakpointRequest(loc);
I'd suggest to move the lines 68,69 after the line 75.
Also, the empty lines 72, 74 are not needed.
I'm not sure, the line 70 with the comment is placed correctly or
needed at all.
This line needs an update of "request1":
107 //request1.addClassFilter("x");
Extra space before '!' and missed space before '{' :
115 if ( !stepCompleted){
Let's simplify/unify the tracing lines below further:
130 System.out.println("Agent: StepEvent: line#=" +
event.location().lineNumber()
131 + " event=" + event);
. . .
141 System.out.println("Agent: BreakpointEvent " +
142 " at " + locStr + " in thread: " + thread);
Something like this would be better:
130 System.out.println("StepEvent at " + event.location());
. . .
141 System.out.println("BreakpointEvent at " + event.location());
The lines 139 and 140 can be removed:
139 ThreadReference thread = event.thread();
140 String locStr = "" + event.location();
Thanks,
Serguei
On 2/13/18 23:01, Paru Somashekar wrote:
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
<mailto: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.
