Skip to content

Implement Copy trait on AstNode#70

Merged
kubouch merged 7 commits into
nushell:mainfrom
WindSoilder:push-srrsqossvquk
May 10, 2026
Merged

Implement Copy trait on AstNode#70
kubouch merged 7 commits into
nushell:mainfrom
WindSoilder:push-srrsqossvquk

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented May 6, 2026

I noted there is a to do comment about AstNode:

// TODO: All nodes with Vec<...> should be moved to their own ID (like BlockId) to allow Copy trait

This pr is going to implement it.

To be honest I'm not really sure if it's intended, because it introduces some indirection. But I think it addresses comment from #68 (comment) to introduce some helper functions to Compiler, so we can use it in a better way.

@WindSoilder WindSoilder requested a review from kubouch May 6, 2026 08:17
@WindSoilder WindSoilder force-pushed the push-srrsqossvquk branch from bbf664c to 91f758c Compare May 6, 2026 08:18
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented May 10, 2026

Seems good. Size of AstNode is unchanged (64 bytes) but it seems to accelerate the parsing a bit (eg. combine_compile_iter1000 is now up to 1ms faster on my machine).

One possible future optimization could be to store those IDs now living in the separate Vecs into either the ast_nodes, or an equivalent of extra_data from the Zig parser, and refer to them as NodeId + length, for example.

@kubouch kubouch merged commit 22df7ad into nushell:main May 10, 2026
4 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.

2 participants