fix unsupported breakpoint at method entry/exit or class#129
Conversation
Add location type check to avoid breakpoint on method/field which will never be hit in current implementation.
update lines to the next line which is valid for breakpoint
| IBreakpoint[] added = manager.setBreakpoints(AdapterUtils.decodeURIComponent(sourcePath), toAdds, bpArguments.sourceModified); | ||
| for (int i = 0; i < bpArguments.breakpoints.length; i++) { | ||
| // check whether the breakpoint has already been processed, otherwise the same breakpoint might be installed more than once. | ||
| // if class name is null, then this breakpoint is not valid, we doesn't need to install it or update hit count for it. |
There was a problem hiding this comment.
we doesn't -> we don't
| if (lines[i] == locator.getLineLocation()) { | ||
| // Add location type check to avoid breakpoint on method/field which will never be hit in current implementation. | ||
| if (locator.getLocationType() == ValidBreakpointLocationLocator.LOCATION_LINE) { | ||
| 10BC0 | if (lines[i] != locator.getLineLocation()) { |
There was a problem hiding this comment.
Just assign anyway to simplify the logic
|
I'm not sure about the purpose of this pull request. Let's sync more tomorrow. |
| // When the final valid line location is same as the original line, that represents it's a valid breakpoint. | ||
| if (lines[i] == locator.getLineLocation()) { | ||
| // Add location type check to avoid breakpoint on method/field which will never be hit in current implementation. | ||
| if (locator.getLocationType() == ValidBreakpointLocationLocator.LOCATION_LINE) { |
There was a problem hiding this comment.
Update the javadoc for getFullyQualifiedName() because the behavior is changed.
There was a problem hiding this comment.
In line 143, we have a todo item that indicates future support of moving breakpoint location to a valid one. Please remove the comments if you just implemented this.
|
|
||
| // Compute the breakpoints that are newly added. | ||
| List<IBreakpoint> toAdd = new ArrayList<>(); | ||
| Set<IBreakpoint> toAdd = new HashSet<>(); |
There was a problem hiding this comment.
BE9DWhy do we need to replace list with set?
There was a problem hiding this comment.
in previous code, the breakpoint line will never be changed, invalid breakpoint will be marked with null class name, thus the breakpoint in toAdd will never be the same, while in new design, the line number may be changed, eg:
- // a
- // b
- c++;
if we use ArrayList, addBreakpointsInternally in the following code will add three same breakpints to this.breakpoints, which will bring duplicate ones in this.breakpoints
| for (int i = 0; i < bpArguments.breakpoints.length; i++) { | ||
| // check whether the breakpoint has already been processed, otherwise the same breakpoint might be installed more than once. | ||
| // if class name is null, then this breakpoint is not valid, we don't need to install it or update hit count for it. | ||
| if (processedBreakpoints.contains(toAdds[i]) || added[i].className() == null) { |
There was a problem hiding this comment.
BreakpointManager checks for duplicate breakpoints. Why do we need to check again?
There was a problem hiding this comment.
eg: the following code
1. // a
2. // b
3. c++;
if we set all breakpoints for the 3 lines, manager.setBreakpoints will return an 3-element array all with the same breakpoint line # 3, when process the first breakpoint on line 1, actually we are installing breakpoint on line 3, so the line 2, and line 3 breakpoints will not be processed again.
| // When the final valid line location is same as the original line, that represents it's a valid breakpoint. | ||
| if (lines[i] == locator.getLineLocation()) { | ||
| // Add location type check to avoid breakpoint on method/field which will never be hit in current implementation. | ||
| if (locator.getLocationType() == ValidBreakpointLocationLocator.LOCATION_LINE) { |
There was a problem hiding this comment.
In line 143, we have a todo item that indicates future support of moving breakpoint location to a valid one. Please remove the comments if you just implemented this.
| // Add location type check to avoid breakpoint on method/field which will never be hit in current implementation. | ||
| if (locator.getLocationType() == ValidBreakpointLocationLocator.LOCATION_LINE) { | ||
| // update lines to the next line which is valid for breakpoint | ||
| lines[i] = locator.getLineLocation(); |
There was a problem hiding this comment.
Modifying what's passed in is not recommended. Please add an override to accept an extra parameter for the modified line numbers.
| // Add location type check to avoid breakpoint on method/field which will never be hit in current implementation. | ||
| if (locator.getLocationType() == ValidBreakpointLocationLocator.LOCATION_LINE) { | ||
| // update lines to the next line which is valid for breakpoint | ||
| lines[i] = locator.getLineLocation(); |
There was a problem hiding this comment.
As discussed, we only support line breakpoint for now.
…to set breakpoint will not report to our lang server immediately
Add location type check to avoid breakpoint on method/field which will never be hit in current implementation.