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/ <http://cr.openjdk.java.net/~lmesnik/8244133/webrev.01/> Leonid > On May 1, 2020, at 4:05 PM, Chris Plummer <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/ >>>> <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> >> >
