Hi
> On May 1, 2020, at 1:12 PM, Chris Plummer <[email protected]> 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.
>
> 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.
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/
>> <http://cr.openjdk.java.net/~lmesnik/8244133/webrev.00/>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8244133
>> <https://bugs.openjdk.java.net/browse/JDK-8244133>
