Re: RFR 8196324: Update FilterMatch & FilterNoMatch to use TestScaffold instead of JDIScaffold

Tue, 13 Feb 2018 14:03:05 -0800

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/>

thanks,
Paru.

Reply via email to