Allow non-contiguous state values. #1279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Requiring the states to be a zero-based and contiguous makes it harder for some pieces to support different sets of different states based on what they contain. For example, an ImportPiece only supports states for splitting the names inside a single combinator when there is more than one combinator. To model that with contiguous states requires the piece to carefully track how state indexes are shifted or splitting pieces into different classes to handle the different sets of states. Instead, with this change, we simply allow the piece to return a list of states that may be non-contiguous. They can then omit states that don't make sense given the piece's contents. This makes ImportPiece simpler and I think will be useful for other pieces that have a variety of ways they can split. Also wrap state values in a class to make things a little clearer and more type safe.
natebosch
approved these changes
Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requiring the states to be a zero-based and contiguous makes it harder for some pieces to support different sets of different states based on what they contain. For example, an ImportPiece only supports states for splitting the names inside a single combinator when there is more than one combinator.
To model that with contiguous states requires the piece to carefully track how state indexes are shifted or splitting pieces into different classes to handle the different sets of states.
Instead, with this change, we simply allow the piece to return a list of states that may be non-contiguous. They can then omit states that don't make sense given the piece's contents. This makes ImportPiece simpler and I think will be useful for other pieces that have a variety of ways they can split.
Also wrap state values in a class to make things a little clearer and more type safe.