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

Add FindBinding which finds binding #146

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Add FindBinding which finds binding #146

merged 3 commits into from
Feb 26, 2020

Conversation

ice1000
Copy link
Contributor

@ice1000 ice1000 commented Feb 22, 2020

Once this is merged, intellij-arend won't compile unless JetBrains/intellij-arend#119 is merged too.

@ice1000 ice1000 requested a review from valis February 22, 2020 06:48
@ice1000 ice1000 self-assigned this Feb 22, 2020
@ice1000 ice1000 closed this Feb 22, 2020
@ice1000 ice1000 deleted the 100 branch February 22, 2020 07:33
@ice1000 ice1000 reopened this Feb 22, 2020
Copy link
Collaborator

@valis valis left a comment

Choose a reason for hiding this comment

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

Casts in FindBinding may produce NPE. If a cast fails, the whole function should fail.

@ice1000
Copy link
Contributor Author

ice1000 commented Feb 26, 2020

Wait, where do you think may cause npe? The the .apply functions are iterations through the bindings, and I return the last binding found because latter binding can shadow prior ones. By implementing the iteration, I'll need to return null when cast fails because there's no more bindings to be iterated. I found this implementation natural, do you have a better idea?

@valis valis merged commit 6631793 into master Feb 26, 2020
Copy link
Contributor Author

@ice1000 ice1000 left a comment

Choose a reason for hiding this comment

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

finished review test

@ice1000 ice1000 added this to the 1.3 milestone Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants