Refactor advancing token to avoid duplication, avoid borrow checker issues #1618
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
alamb
commented
Dec 24, 2024
| @@ -1276,7 +1276,9 @@ impl<'a> Parser<'a> { | |||
|
|
|||
| let dialect = self.dialect; | |||
|
|
|||
| let (next_token, next_token_index) = self.next_token_ref_with_index(); | |||
| self.advance_token(); | |||
There was a problem hiding this comment.
This has two benefits:
- I think it is more explicit
- It doesn't not result in a
mutborrow ofself(this is the key difference)
| @@ -3641,22 +3670,31 @@ impl<'a> Parser<'a> { | |||
| #[must_use] | |||
| pub fn parse_keyword(&mut self, expected: Keyword) -> bool { | |||
| if self.peek_keyword(expected) { | |||
| self.next_token_ref(); | |||
| self.advance_token(); | |||
There was a problem hiding this comment.
I think this is nicer too as it makes clear the token is unused (it is just being advanced)
| /// | ||
| /// # Notes: | ||
| /// OK to call repeatedly after reaching EOF. | ||
| pub fn next_token_ref_with_index(&mut self) -> (&TokenWithSpan, usize) { |
There was a problem hiding this comment.
Note thse APIs were added in #1587 (so not yet released) so this isn't a breaking API change
Co-authored-by: Andrew Lamb <[email protected]>
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.
Tokens as much #1558@davisp made some great improvements to the
ParserAPI to avoid copies, but now the API for managing tokens is a bit unwieldy. Before we release a new version I would like to improve the APISpeicfically some of the APIs take a
&mut selfto advanceindexbut return a read only reference to the token. This means the borrow checker is overly stringent and will sometimes think the parser has a mutable borrow outstanding when it doesnt.I think we can make the APIs easier to use (and thus more likely to use) with a bit of finagling and documentation
See definitions of previous/current/next on #1617
I am hoping that we can get the API in shape a bit more and then deprecate some of the older style APIs.
Changes:
advance_token()to advance to the next token and reduce redundancyget_current_token(),get_next_token()get_prev_tokencurrent_token_indexnext_token_ref_with_indexandnext_token_refin favor of the above