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

[Paru Somashekar]

Sound great, thanks Serguei.

thanks,
Paru.

On 2/14/18, 4:50 PM, serguei.spitsyn at oracle.com wrote:
> Paru,
>
> Thank you for the update.
>
> Just noticed one more thing in both files - extra space before the 
> parameter:
> +            addListener (this);
>
> Otherwise, it looks good.
> No need in another webrev.
>
> Thanks,
> Serguei
>
>
>
> On 2/14/18 16:39, Paru Somashekar wrote:
>> 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 
>> <mailto: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/0c929c27/attachment-0001.html>