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

Handle Map.Entry properly in import removal, and remove duplicated logic #390

Merged
merged 6 commits into from
Feb 3, 2025

Conversation

kelloggm
Copy link
Collaborator

@kelloggm kelloggm commented Jan 23, 2025

Prior to this PR, there were two pieces of code in Specimin that were both trying to handle removing extraneous imports:

  • some code in PrunerVisitor, and
  • the UnusedImportRemoverVisitor class

I deleted the logic in PrunerVisitor, which was more complex but had the same overall impact. I enhanced the logic in UnusedImportRemoverVisitor to fix the bug evinced by the new test case, wherein the import java.util.Map was incorrectly removed. I also had to add some continue statements elsewhere in PrunerVisitor; that part makes me nervous, because a seemingly-unrelated test failed without them but did not fail until I removed the logic in PrunerVisitor. However, I much prefer this design and don't have time to dig deep into that, so I'm okay with ignoring that weirdness for now: the new continue statements are pretty clearly correct (it's not sensible to keep going through the loop body's logic if we've already decided to remove something).

I expect CI to fail, because this PR should resolve some compilation issues in plume-util. I'll update the "passed" baseline number once I know what the new result is.

Copy link
Collaborator

@M3CHR0M4NC3R M3CHR0M4NC3R left a comment

Choose a reason for hiding this comment

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

I am able to reproduce these results on my local machine as well. The accuracy was also reported at 91.24. The accuracy file will conflict with the other pull requrests so that will need to be checked again afterwards. Otherwise this looks good to me.

@kelloggm
Copy link
Collaborator Author

kelloggm commented Feb 3, 2025

I am able to reproduce these results on my local machine as well. The accuracy was also reported at 91.24

Thanks for confirming. This is good to go, in that case.

@kelloggm kelloggm merged commit 3faf154 into main Feb 3, 2025
2 of 3 checks passed
@kelloggm kelloggm deleted the arraymapclear branch February 3, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants