Re: 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.spit...@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/ 



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/ 



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 

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

[serguei.spit...@oracle.com]

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

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



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/ 



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 

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

[serguei.spit...@oracle.com]

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

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

[Paru Somashekar]

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/ 



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/ 
 



thanks,
Paru.

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

[serguei.spit...@oracle.com]

  
  
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/
  
  
  
  thanks,
  
  Paru.
  


  

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

[Paru Somashekar]

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/ 



thanks,
Paru.