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

Rename glue and refactor push_tree #106481

Closed
wants to merge 2 commits into from
Closed

Rename glue and refactor push_tree #106481

wants to merge 2 commits into from

Conversation

notJoon
Copy link
Contributor

@notJoon notJoon commented Jan 5, 2023

Renamed the glue function to check_is_multiple_char_token and added some comments.

And I think the push_tree function could only check the !Self::try_glue_to_last condition, so I refactored it.

thanks

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 5, 2023
Comment on lines +713 to +716
/// Checks if the current token is a multiple-character token (or glued like `==`, `<=`, `...`)
/// and returns the corresponding `TokenKind` if it is.
///
/// if the current tokens is already complete, returns `None`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment explains what this method is doing properly.

What this is doing is checking if the current token can be combined with the joint token to turn into a multiple-character token.

/// and returns the corresponding `TokenKind` if it is.
///
/// if the current tokens is already complete, returns `None`.
pub fn check_is_multiple_char_token(&self, joint: &Token) -> Option<Token> {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the name glue. Adding a doc comment is fine, but the new name doesn't really explain that we're trying to combine self and joint into a new "glued"/merged/combined/joined token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly it doesn't seem very practical. Thank you for your feedback.

@notJoon notJoon closed this Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants