Thank you for review and feedback.
Leonid
On 5/1/20 7:45 PM, serguei.spit...@oracle.com wrote:
Hi Leonid,
I'd suggest to align parameters and remove two commented lines:
90 protected final BreakpointRequest settingBreakpoint(ThreadReference
thread,
91 ReferenceType
testedClass,
92 String methodName,
93 String bpLine,
94 String property)
. . .
141 protected final void getEventSet() throws JDITestRuntimeException {
142 try {
143 // jdiHelper.log2(" eventSet =
eventQueue.remove(waitTime);");
144 eventSet = eventQueue.remove(waitTime);
145 if (eventSet == null) {
146 throw new JDITestRuntimeException("** TIMEOUT while waiting
for event **");
147 }
148 // jdiHelper.log2(" eventIterator =
eventSet.eventIterator;");
Otherwise, looks good.
No need in new webrev if you fix it.
Thanks,
Serguei
On 5/1/20 16:51, Leonid Mesnik wrote:
Yes, the protected should be enough. I've made all non constant
fields and methods protected and remove comment.
The updated webrev (JDIBase only)
http://cr.openjdk.java.net/~lmesnik/8244133/webrev.01/
Leonid
On May 1, 2020, at 4:05 PM, Chris Plummer <chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com>> wrote:
On 5/1/20 3:42 PM, Leonid Mesnik wrote:
Hi
On May 1, 2020, at 1:12 PM, Chris Plummer
<chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote:
Hi Leonid,
Overall looks good.
How did you verify that all settingBreakpoint() implementations
were the same? How did you find that some of the
breakpointForCommunication() implementations were different?
Unfortunately I didn't find a good way to check that all these
method are exactly the same. I just go through diff without lines
common for all methods and verify that nothing unexpected was removed.
Using this I found the different breakpointForCommunication() and
settingBreakpoint() implementation. I found 2 variants of
settingBreakpoint(). The one which set breakpointLocation (or
location) and one which don't set.
Additionally I grepped new files to check that lines like ' static
final int PASSED' and other known patterns are removed.
Ok.
A few minor things in JDIBase:
108 throws JDITestRuntimeException {
I think it would look better with the { on a new line.
80 // used by tests with breakpoint communication
81 public EventRequestManager eventRManager;
82 public EventQueue eventQueue;
83 public EventSet eventSet;
84 public EventIterator eventIterator;
85
86 // additional fields initialized during breakpoint
communication
87 public Location breakpLocation = null;
88 public BreakpointEvent bpEvent;
Do these fields need to be public and not just protected? Public
fields in java are not the norm.
91 /*
92 * private BreakpointRequest
settingBreakpoint(ThreadReference, ReferenceType,
93 * String, String, String)
94 *
95 * It sets up a breakpoint at given line number within a
given method in a given class
96 * for a given thread.
97 *
98 * Return value: BreakpointRequest object in case of success
99 *
100 * JDITestRuntimeException in case of an Exception
thrown within the method
101 */
123 int n =
124 ((IntegerValue)
testedClass.getValue(testedClass.fieldByName(bpLine))).value();
I'm not sure why the comment includes the prototype, but in any
case it is out of date. I suggest just removing it.
Also, there are double-spaces in lines 98 and 100.
I could make fields protected and remove comments. But I want to
review base class and might be make more refactoring. Also there
are a couple of tests with strange formatting, so I am going to add
them separately.
I just don't want to put all this in single change. So diff
contains mostly removal of same lines and it is easier to check it.
But I'm just referring to the one copy in JDIBase.java. It seems
getting the format and commenting of this entirely new class is
something to do now, not at some later point when you cleanup other
tests.
Also, the fields were not public before being moved to JDIBase, they
were package private. So unless there is a reason for them to be
public, I'm not sure why you would not be more restrictive as long
as it does not impact the tests. Do the tests need for the fields to
be public or else would need to be modified?
thanks,
Chris
Leonid
thanks,
Chris
On 4/29/20 4:45 PM, Leonid Mesnik wrote:
Hi
Could you please review following fix which remove code
duplication by moving methods BreakpointRequest
settingBreakpoint, getEventSet, breakpointForCommunication and
log1,2,3 in base class.
The fix is huge but pretty straight forward, I tried to keep
changes as small as possible so most tests just changed to
extends JDIBase and corresponding methods and fields were deleted.
Some tests used slightly different 'breakpointForCommunication()'
implementation so they override it now.
Some tests contain change like this:
- eventRequest1 = eventRManager.createBreakpointRequest(location);
+ eventRequest1 =
eventRManager.createBreakpointRequest(breakpLocation);
it is because they had 'location' and not 'breakpLocation' which
is set by settingBreakpoint(...). So just merged location and
breakpLocation.
All tests passed, so the main goal is to ensure that changes
don't introduce false positive results ignoring exit codes and
statuses.
I quickly verified that updated files don't contain declarations
of variable like PASSED, testExitCode and other.
The JDIBase and all tests could be improved, also there are still
a lot of tests which could use it as a base as well as other base
classes for idi debuggers. However I want to keep this fix as
simple as possible.
werbev: http://cr.openjdk.java.net/~lmesnik/8244133/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8244133
