Skip to content

Teach TokenSpecSet about experimental features #2211

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 5 commits into from
Sep 19, 2023

Conversation

hamishknight
Copy link
Contributor

Pass down the current set of experimental features to TokenSpecSets, and have generated TokenSpecSets automatically account for experimental features by returning nil if a given feature isn't active. Should be NFC.

@hamishknight
Copy link
Contributor Author

hamishknight commented Sep 18, 2023

I can't say I love having to pass down experimental features to each TokenSpecSet, but I think it's ultimately better having them automatically handle experimental features instead of needing ad-hoc checks in the parser which, as demonstrated recently, can be error prone

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.

Thanks for doing this. I agree that passing the experimental feature list to TokenSpecSet isn’t great but it’s definitely less error-prone.

I measured performance and couldn’t see any regression when parsing NetNewsWire.

Pass down the current set of experimental features
to TokenSpecSets, and have generated TokenSpecSets
automatically account for experimental features by
returning `nil` if a given feature isn't active.
We now check these with the spec set.
@hamishknight
Copy link
Contributor Author

@swift-ci please test

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Sep 18, 2023
@hamishknight
Copy link
Contributor Author

@swift-ci please test macOS

@hamishknight hamishknight merged commit fb95cbb into swiftlang:main Sep 19, 2023
@hamishknight hamishknight deleted the up-to-spec branch September 19, 2023 09:32
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