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.

Sign up for GitHub

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

Jump to bottom

feat(ast, syntax, ast_tools): Introduce GetNodeId trait. #5776

Draft
wants to merge 2 commits into
base: 09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types
Choose a base branch
from
Draft

feat(ast, syntax, ast_tools): Introduce GetNodeId trait. #5776

wants to merge 2 commits into from

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Sep 14, 2024 •
edited
Loading

Part of #5689

@graphite-app Graphite App
Copy link

graphite-app bot commented Sep 14, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@rzvxa rzvxa mentioned this pull request Sep 14, 2024
Merged
@rzvxa Graphite App
Copy link
Contributor Author

rzvxa commented Sep 14, 2024 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added A-ast Area - AST A-ast-tools Area - AST tools labels Sep 14, 2024
@rzvxa rzvxa mentioned this pull request Sep 14, 2024
Draft
@rzvxa rzvxa marked this pull request as ready for review September 14, 2024 22:36
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Sep 14, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #5776 will not alter performance

Comparing 09-15-feat_ast_syntax_ast_tools_introduce_getnodeid_trait (97285a5) with 09-15-feat_ast_add_a_dummy_node_id_field_to_all_visitable_ast_types (f5344f9)

Summary

✅ 29 untouched benchmarks

overlookmotel
overlookmotel reviewed Sep 16, 2024
View reviewed changes
crates/oxc_syntax/src/node.rs Outdated
@@ -7,6 +7,11 @@ use oxc_span::{cmp::ContentEq, hash::ContentHash};
#[cfg(feature = "serialize")]
use serde::{Serialize, Serializer};

pub trait GetNodeId {
fn node_id(&self) -> NodeId;
fn node_id_mut(&mut self) -> &mut NodeId;
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in previous PR, I'm not sure we should make node_id fields mutable, so I'd suggest removing this method.

Also, possibly we want to implement GetNodeId on AstKind, which won't be possible if GetNodeId::node_id_mut exists.

@rzvxa rzvxa force-pushed the 09-15-feat_ast_syntax_ast_tools_introduce_getnodeid_trait branch from ccc4a6a to 97285a5 Compare September 16, 2024 18:55
@Boshen Boshen marked this pull request as draft November 21, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overlookmotel overlookmotel overlookmotel left review comments

@Boshen Boshen Awaiting requested review from Boshen

Assignees
No one assigned
Labels
A-ast Area - AST A-ast-tools Area - AST tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rzvxa @overlookmotel