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

Fix #75 by updating TOP method node id and returned overridden methods #76

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

nimakarimipour
Copy link
Member

@nimakarimipour nimakarimipour commented Sep 20, 2022

This PR resolves #75 by updating TOP node id and index. Also making sure that getClosestSuperMethod() only returns methodNode for discovered nodes. (Declared in module)

@nimakarimipour nimakarimipour added the bug Something isn't working label Sep 20, 2022
@nimakarimipour nimakarimipour self-assigned this Sep 20, 2022
@nimakarimipour nimakarimipour changed the title Fix #75 by updating TOP method node id. Fix #75 by updating TOP method node id and returned overridden methods Sep 20, 2022
Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Looks good to me except for one question:

  • Here, in MethodDeclarationTree.addNodeByLine, where does the id come from? Is that from the TSV files? And if so, is that list one-indexed or zero-indexed? Because this change seems like it could cause MethodNode.TOP, which I believe is an artificial node, with the first node in the TSV.

That said, I pulled in this change and managed to get past the crash from before, but I am having a separate issue. Will report on the tracker.

@nimakarimipour
Copy link
Member Author

Looks good to me except for one question:

  • Here, in MethodDeclarationTree.addNodeByLine, where does the id come from? Is that from the TSV files? And if so, is that list one-indexed or zero-indexed? Because this change seems like it could cause MethodNode.TOP, which I believe is an artificial node, with the first node in the TSV.

That said, I pulled in this change and managed to get past the crash from before, but I am having a separate issue. Will report on the tracker.

@lazaroclapp The list is one-indexed and the ids come from the tsv file. It is guaranteed that ids starts from 1. Please let me know if you approve this and I will land this. Thank you.

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

SGTM!

Edit: For my own clarity, that id originally comes from ScannerContext.getNextMethodId() in the type-annotator-scanner, which starts with a zero id internally, but pre-increments it before returning it every time, so it does count 1-indexed.

@nimakarimipour nimakarimipour merged commit c095754 into master Sep 20, 2022
@nimakarimipour nimakarimipour deleted the nimak/issue-75 branch September 20, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on retrieving undiscovered method node.
2 participants