DataStack to use a single unified enum stack & document memory optimizations#50
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a memory optimization to the generated parser by replacing multiple typed stacks and a tag stack with a single unified data stack containing a newly introduced unified enum. This change simplifies stack operations like clear, split_off, truncate, and append into single vector operations, and the documentation is updated to recommend wrapping large types in a Box to minimize enum variant sizes. Feedback on the changes highlights two critical compilation issues in the code generator: first, several streams (such as debug_tag_check_stream and extract_data_stream) are defined inside an if block but are referenced in the corresponding else block; second, the Empty variant is conditionally generated, yet unconditionally referenced by methods like push_empty and pop_start, which will cause compilation failures when no empty tags are used.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| } | ||
|
|
||
| if rule.is_used { |
There was a problem hiding this comment.
The variables debug_tag_check_stream, extract_data_stream, alias_stream, and custom_reduce_action_stream are defined inside the if rule.is_used block, but they are referenced in the else block (around line 1384). This will cause a compilation error in the generator. Defining them as empty TokenStreams before the if block resolves this issue.
let debug_tag_check_stream = TokenStream::new();
let extract_data_stream = TokenStream::new();
let alias_stream = TokenStream::new();
let custom_reduce_action_stream = TokenStream::new();
if rule.is_used {| if empty_tag_used { | ||
| tag_definition_body_stream.extend(quote! { | ||
| #empty_tag_name, | ||
| variants.extend(quote! { | ||
| Empty, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The Empty variant is conditionally added to the data enum only if empty_tag_used is true. However, generated methods like push_empty and pop_start unconditionally reference #data_enum_typename::Empty. If a grammar has no empty tags used, the generated parser will fail to compile due to the missing Empty variant. The Empty variant should be unconditionally included in the enum.
variants.extend(quote! {
Empty,
});
This PR refactors the generated parser's
DataStackimplementation from using multiple type-specific vectors and a tracking tag vector to a single, unified vector containing all token/rule variants wrapped in a single generated enum. This significantly simplifies the generated parser code, reduces generated code size, and improves runtime management of stack operations.Additionally, this PR adds a documentation section on optimizing memory usage for large rule types.
Key Changes
emit.rs):__stack0,__stack1, etc.) and the tracking tag vector (__tags) with a single enum (e.g.,GrammarData) and a single stack vector (__stack: Vec<GrammarData>).DataStacktrait methods (pop,clear,reserve,split_off,truncate,append) to directly delegate to the single underlying__stackvector.pop_startto pop the trailing EOF (Emptytoken) before extracting the final parse result.variant_names_for_nontermandruletype_variant_map) to store and returnIdentdirectly instead of wrapping them inOption<Ident>, mapping untyped symbols to theEmptyvariant.SYNTAX.md):RuleTypesection. It clarifies that since all parser semantic values are stored in a single unified enum, wrapping large data types in aBox(e.g.,Box<MyLargeStruct>) is recommended to prevent stack memory bloat.Verification