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.

Sign up for GitHub

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

Jump to bottom

Fix for Conditional Breakpoint not working in File class. #476

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Fix for Conditional Breakpoint not working in File class. #476

merged 3 commits into from
Sep 24, 2024

Conversation

SougandhS
Copy link
Contributor

@SougandhS SougandhS commented Aug 27, 2024 •
edited
Loading

What it does

Handles all the exception caused while evaluating conditional breakpoint java.io.File class
Fix for -> #273

How to test

Drop a valid conditional breakpoint inside java.io.File class and run in debug mode

Author checklist

@iloveeclipse
Copy link
Member

iloveeclipse commented Aug 29, 2024 •
edited
Loading

Would be very nice to have a regression test added, also please make sure there is one commit with proper commit message explaining the fix.

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch from 7dd2292 to b2bfc40 Compare September 2, 2024 05:33
iloveeclipse
iloveeclipse previously requested changes Sep 2, 2024
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

Thanks for the new test, there are few smaller issues to fix, see comments above

org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java Outdated Show resolved Hide resolved
org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java Outdated Show resolved Hide resolved
org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java Outdated Show resolved Hide resolved
org.eclipse.jdt.debug.tests/testprograms/FileConditionSnippet2.java Outdated Show resolved Hide resolved
...tests/tests/org/eclipse/jdt/debug/tests/breakpoints/ConditionalBreakpointsWithFileClass.java Show resolved Hide resolved
@iloveeclipse
Copy link
Member

Also please rebase (do not merge with!) on latest master branch state.

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch from d67745e to 92c6b47 Compare September 2, 2024 10:16
@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 6 times, most recently from d5b431b to 5300310 Compare September 3, 2024 13:47
@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 12 times, most recently from 1ea982a to ebb0044 Compare September 6, 2024 17:28
@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 4 times, most recently from 5ef1ac9 to e80b1dd Compare September 14, 2024 13:52
@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 6 times, most recently from 315815c to 71a83de Compare September 24, 2024 05:59
@SougandhS
Copy link
Contributor Author

Hi @iloveeclipse, done all the changes.

Sorry for me not answering timely, I'm currently working 2 days in a week only and have almost no free time for PR reviews.

Okay @iloveeclipse ,no problem.

Hi @jukzi ,could you pls review this ?

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch from 71a83de to 7b5ad57 Compare September 24, 2024 09:52
jukzi
jukzi reviewed Sep 24, 2024
View reviewed changes
...lipse.jdt.debug/eval/org/eclipse/jdt/internal/debug/eval/ast/engine/ASTEvaluationEngine.java Outdated
if (((IJavaVariable) event.getSource()).getDebugTarget()
.equals(getDebugTarget())) {
if (event.getSource() instanceof IJavaVariable && event.getKind() == DebugEvent.CHANGE) {
if (((IJavaVariable) event.getSource()).getDebugTarget().equals(getDebugTarget())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid typecast with instanceof pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change those now.

jukzi
jukzi reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

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

With all that code reformat it is hard to identify in the review what the actual change is. but overall it looks sane.

@jukzi jukzi dismissed iloveeclipse’s stale review September 24, 2024 10:08

dismiss stale review

@jukzi
Copy link
Contributor

jukzi commented Sep 24, 2024

Is it possible with adequate effort to split the automated code reformating in a separate commit (Ok to have it in the same PR though) to make the review easier? For that you could just reformat the classes in question and rebase this PR on top of it.

@SougandhS
Copy link
Contributor Author

Is it possible with adequate effort to split the automated code reformating in a separate commit (Ok to have it in the same PR though) to make the review easier? For that you could just reformat the classes in question and rebase this PR on top of it.

Sure will do that.

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 2 times, most recently from 5868e50 to eb547ad Compare September 24, 2024 10:44
@jukzi
Copy link
Contributor

jukzi commented Sep 24, 2024

@RoiSoleil can you test if this PR solves the issue you raised?

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch 2 times, most recently from f19b1b2 to 292a24f Compare September 24, 2024 11:00
@SougandhS
Copy link
Contributor Author

SougandhS commented Sep 24, 2024 •
edited
Loading

With all that code reformat it is hard to identify in the review what the actual change is. but overall it looks sane.

Is it possible with adequate effort to split the automated code reformating in a separate commit (Ok to have it in the same PR though) to make the review easier? For that you could just reformat the classes in question and rebase this PR on top of it.

Actually the changes are only in ASTEvaluationEngine.createExpressionFromAST(String,EvaluationSourceGenerator,CompilationUnit) line: 601
(Added additional problem id filter for Ambiguity)

ConditionalBreakpointHandler.breakpointHit(IJavaThread,IJavaBreakpoint) line: 222
(Getting project details if provided with BinaryType)

Rest of changes are only for test cases.
Made separate commits like you suggested

@SougandhS SougandhS requested a review from jukzi September 24, 2024 11:33
@RoiSoleil
Copy link

@RoiSoleil can you test if this PR solves the issue you raised?

It's working with the PR. Ok for me.

@SougandhS SougandhS force-pushed the Conditional_BreakPoint_File_Fix branch from 292a24f to 1e424e0 Compare September 24, 2024 13:49
@jukzi
Copy link
Contributor

jukzi commented Sep 24, 2024

looks ok, was tested => lets merge

@jukzi jukzi merged commit 7394536 into eclipse-jdt:master Sep 24, 2024
10 checks passed
@SougandhS
Copy link
Contributor Author

Thank you @jukzi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveeclipse iloveeclipse Awaiting requested review from iloveeclipse

@jukzi jukzi Awaiting requested review from jukzi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SougandhS @iloveeclipse @jukzi @RoiSoleil