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

[Paru Somashekar]

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.spitsyn at 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.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at 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.
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180214/202ebcaf/attachment.html>