Skip to content

[SwiftLexicalLookup][GSoC] New lookup method signature and Identifier initializer changes #2789

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

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

MAJKFL
Copy link
Contributor

@MAJKFL MAJKFL commented Aug 8, 2024

This PR includes changes suggested by @ahoppen under #2748 that didn't make it before merging.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

@MAJKFL
Copy link
Contributor Author

MAJKFL commented Aug 9, 2024

  1. I removed LookupName.refersTo and renamed ScopeSyntax.checkName to ScopeSyntax.checkIdentifier for extra clarity.
  2. As far as I understand, it's not used anywhere in the compiler right now. Though there's this weird top level code behavior in playgrounds I described here. To be honest, memberBlock configuration wouldn't be perfect either, as it doesn't extract names from guard. I think everything will become clearer once we start testing against the compiler and as for now I think I'd like to leave the implementation as is so we can later iterate on it.
  3. I think I might not fully understand this one. I added a test case with your example here. As far as I understand the result should be empty, or am I missing something?
  4. I wanted to provide extra flexibility for sequential scopes to be able to use any result kinds, but I see now it was probably superfluous. I added a new LookupResult.getResult static method that creates a result specific for the provided scope kind.

@MAJKFL MAJKFL requested a review from ahoppen August 9, 2024 15:04
@ahoppen
Copy link
Member

ahoppen commented Aug 9, 2024

  1. I removed LookupName.refersTo and renamed ScopeSyntax.checkName to ScopeSyntax.checkIdentifier for extra clarity.

Thanks 👍🏽

  1. As far as I understand, it's not used anywhere in the compiler right now. Though there's this weird top level code behavior in playgrounds I described here. To be honest, memberBlock configuration wouldn't be perfect either, as it doesn't extract names from guard. I think everything will become clearer once we start testing against the compiler and as for now I think I'd like to leave the implementation as is so we can later iterate on it.

OK, just wanted to make sure we are aware of it and don’t carry around old cruft just because nobody knows what it might be used for 👍🏽

  1. I think I might not fully understand this one. I added a test case with your example here. As far as I understand the result should be empty, or am I missing something?

Oh sorry, I missed that test case. Looks good.

  1. I wanted to provide extra flexibility for sequential scopes to be able to use any result kinds, but I see now it was probably superfluous. I added a new LookupResult.getResult static method that creates a result specific for the provided scope kind.

That’s a good step. I still think that it’s worth exploring to unify the fromFileScope and fromScope cases.

@ahoppen
Copy link
Member

ahoppen commented Aug 9, 2024

@swift-ci Please test

@DougGregor DougGregor merged commit 0b324f8 into swiftlang:main Aug 12, 2024
3 checks passed
@MAJKFL MAJKFL deleted the guard-sequential-suggeested-changes branch October 16, 2024 11:23
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.

3 participants