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

@WindSoilder WindSoilder commented May 6, 2026

Copy link
Copy Markdown
Contributor

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

kubouch commented May 10, 2026

Copy link
Copy Markdown
Contributor

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