Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve inline breakpoint discovery when expression is multiline. Fix #521 #522

Merged

Conversation

gayanper
Copy link
Contributor

@gayanper gayanper commented Dec 2, 2023

When checking for lambda expression visible in same line, instead of matching the lambda line to current source line, this fix checks if the lambda is related to the current source line as part of a expression by traversing the parents unit we find the source line.

// the lambda doesn't belong to current line at all
break;
}
node = node.getParent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recursively checking the parent is not efficient, any way to return earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well do you see that we could run forever where theline < sourceLine will be never reached ? Otherwise i think in most cases we will end from the first iteration where line == sourceLine right ? Only in a multiline scenario we will traverse to see if the lambda we found belongs to the same line we add a breakpoint. And I also assume this logic will be executed only for breakpoints that are installed on the source line isn't it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could follow Eclipse’s behavior for breakpoint support on the multiline expression, which allows setting a normal line breakpoint on it directly. This is simpler than the current approach.

I found that we only need a small change on the constructor of BreakpointLocationLocator. We can use the constructor public ValidBreakpointLocationLocator(CompilationUnit compilationUnit, int lineNumber, boolean bindingsResolved, boolean bestMatch, int offset, int end) to initialize the breakpoint validator. This can recognize the multiline lambda on the selected line.

BreakpointLocationLocator locator = new BreakpointLocationLocator(astUnit,
sourceLine, true, true);

// passing the offset to the constructor, it can recognize the multline lambda expression well
BreakpointLocationLocator locator = new BreakpointLocationLocator(astUnit,
                        sourceLine, true, true, astUnit.getPosition(sourceLine, 0), 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the fix

@gayanper gayanper force-pushed the multi-line-breakpoint-discovery branch 2 times, most recently from c4dfc7d to f50b519 Compare December 5, 2023 22:05
@gayanper gayanper force-pushed the multi-line-breakpoint-discovery branch from f50b519 to d8845eb Compare December 5, 2023 22:08
@gayanper gayanper force-pushed the multi-line-breakpoint-discovery branch from 191b7a5 to 9337cea Compare December 12, 2023 21:14
Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contribution.

@testforstephen testforstephen merged commit 9bdd997 into microsoft:main Dec 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants