use separated Vec for each rule_type instead of TokenData#21
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the parser data stack implementation by replacing the enum-based TokenData approach with separate Vec stacks for each rule type. The change introduces a new DataStack trait and implements a struct-based data stack with separate vectors for different token types and a tag stack to track which stack is used for each element.
Key Changes
- Replaces
TokenDataenum withDataStacktrait and struct-based implementation - Introduces separate vectors for each rule type instead of a single unified data stack
- Adds tag tracking system to identify which stack each element belongs to
- Modifies parser contexts to use the new data stack structure
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rusty_lr_parser/src/emit.rs | Implements the new data stack code generation, replacing token data enum generation with separate stack structures |
| rusty_lr_core/src/parser/nondeterministic/node.rs | Updates node structure to use DataStack trait instead of TokenData |
| rusty_lr_core/src/parser/nondeterministic/error.rs | Updates error types to use DataStack trait |
| rusty_lr_core/src/parser/nondeterministic/context.rs | Refactors context implementation for new data stack approach |
| rusty_lr_core/src/parser/mod.rs | Adds new data_stack module |
| rusty_lr_core/src/parser/deterministic/error.rs | Updates error types for deterministic parser |
| rusty_lr_core/src/parser/deterministic/context.rs | Updates deterministic context for new data stack |
| rusty_lr_core/src/parser/data_stack.rs | Defines the new DataStack trait interface |
| rusty_lr_core/src/nonterminal.rs | Removes the old TokenData trait |
Comments suppressed due to low confidence (1)
rusty_lr_parser/src/emit.rs:62
- [nitpick] The variable name
data_stackshould be consistent with the new naming convention. Consider usingdataor a more specific name to match the DataStack terminology.
stream.extend(
| let mut debug_tag_check_stream = TokenStream::new(); | ||
| let mut extract_location_stream = TokenStream::new(); | ||
| let mut stack_mapto_map = std::collections::BTreeMap::new(); | ||
| for (idx_from_back, token) in rule.tokens.iter().rev().enumerate() { |
There was a problem hiding this comment.
[nitpick] The variable name idx_from_back could be more descriptive. Consider using reverse_index or token_index_from_end to better convey its purpose.
| for (idx_from_back, token) in rule.tokens.iter().rev().enumerate() { | |
| for (token_index_from_end, token) in rule.tokens.iter().rev().enumerate() { |
| let truncate_data_stack = if tokens_len > 1 { | ||
| quote! { __data_stack.truncate(__data_stack.len() - #tokens_len + 1); } | ||
| let len = rule.tokens.len(); | ||
| debug_assert!(len > 0); |
There was a problem hiding this comment.
This debug assertion appears to assume that rule.tokens.len() > 0, but the surrounding code handles the case where len could be 0. Consider removing this assertion or adding a comment explaining why this condition should always be true.
| debug_assert!(len > 0); |
| debug_assert!(count > 0); | ||
| stack_truncate_stream.extend(quote! { | ||
| __data_stack.#stack_name.truncate(__data_stack.#stack_name.len() - #count); | ||
| }); |
There was a problem hiding this comment.
Similar to the previous assertion, this assumes count > 0 but the code may need to handle cases where count could be 0. Consider adding validation or a comment explaining this assumption.
| debug_assert!(count > 0); | |
| stack_truncate_stream.extend(quote! { | |
| __data_stack.#stack_name.truncate(__data_stack.#stack_name.len() - #count); | |
| }); | |
| if count > 0 { | |
| stack_truncate_stream.extend(quote! { | |
| __data_stack.#stack_name.truncate(__data_stack.#stack_name.len() - #count); | |
| }); | |
| } |
| self_.reserve(capacity); | ||
| self_ | ||
| } | ||
|
|
There was a problem hiding this comment.
The split_off method lacks documentation explaining what the at parameter represents and the behavior of the method. Consider adding a docstring to clarify its usage.
| /// Splits the data stack into two at the given index. | |
| /// | |
| /// After calling this method, the original stack will contain elements `[0, at)`, | |
| /// and the returned stack will contain elements `[at, len)`. | |
| /// | |
| /// # Parameters | |
| /// - `at`: The index at which to split the stack. | |
| /// | |
| /// # Returns | |
| /// A new data stack containing the elements from the original stack starting at `at`. |
| self_ | ||
| } | ||
|
|
||
| fn split_off(&mut self, at: usize) -> Self; |
There was a problem hiding this comment.
The append method should be documented to explain that it appends the contents of other to self and what happens to the other instance after the operation.
| fn split_off(&mut self, at: usize) -> Self; | |
| fn split_off(&mut self, at: usize) -> Self; | |
| /// Appends the contents of `other` to `self`. | |
| /// | |
| /// After the operation, `other` is emptied and left in its default state. |
See PR overview by copilot below