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

[rust] add a converter from concrete nodes to generic Node #1303

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Aug 22, 2023

The motivating example for this change is a visitor that wants to maintain parent node information as it traverses the AST. The type of the stack maintained during the traversal should be Vec<Node>, but there's no way from the concrete nodes to upcast to the Node enum.

Copy link
Collaborator

@HParker HParker left a comment

Choose a reason for hiding this comment

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

This pattern is also common in the C code, so makes sense you sometimes want a generic node.

@HParker
Copy link
Collaborator

HParker commented Aug 22, 2023

@froydnj What do you think about a test here as well?

@froydnj
Copy link
Contributor Author

froydnj commented Aug 22, 2023

@HParker done! (Mostly just to make sure that as_node() compiles correctly.)

@HParker
Copy link
Collaborator

HParker commented Aug 22, 2023

Mind squashing into logical commits as well? Then I think we are good to merge.

Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Yeah just squash and then we can merge

@froydnj froydnj force-pushed the froydnj-generic-node-conversion branch from c76afd8 to 02eb4d4 Compare August 22, 2023 22:20
@froydnj
Copy link
Contributor Author

froydnj commented Aug 22, 2023

Squashed, thanks!

@HParker HParker merged commit 7b2f516 into ruby:main Aug 23, 2023
45 checks passed
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