split location stack for reduce action#20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the reduction action mechanism in the LR parser by splitting location tracking from data stacks and optimizing the stack management. The changes eliminate the need for reverse-order stack copying by modifying how reduce actions access token data and locations.
- Removes the sorted rules structure and implements direct rule lookup
- Splits data and location stacks in reduce actions for better performance
- Adds optimized node preparation methods to avoid reverse order copying
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rusty_lr_parser/src/grammar.rs | Removes sorted rules storage and implements direct rule lookup |
| rusty_lr_parser/src/emit.rs | Refactors reduce action code generation to use separate data/location stacks |
| rusty_lr_core/src/parser/nondeterministic/node.rs | Adds capacity management methods for node optimization |
| rusty_lr_core/src/parser/nondeterministic/context.rs | Implements prepare_reduce_node method and updates reduce logic |
| rusty_lr_core/src/parser/deterministic/context.rs | Updates deterministic parser to use new reduce action signature |
| rusty_lr_core/src/nonterminal.rs | Changes TokenData trait to use separate data/location stacks |
| pub fn get_rule_by_id(&self, rule_idx: usize) -> Option<(&NonTerminalInfo, usize)> { | ||
| let &(nonterm_idx, rule_local_id) = self.rules_sorted.get(rule_idx)?; | ||
| Some((&self.nonterminals[nonterm_idx], rule_local_id)) | ||
| pub fn get_rule_by_id(&self, mut rule_idx: usize) -> Option<(&NonTerminalInfo, usize)> { |
There was a problem hiding this comment.
The linear search through nonterminals could be inefficient for grammars with many nonterminals. Consider maintaining a mapping from rule index to (nonterminal, rule_local_id) for O(1) lookup.
| __data_stack: &mut Vec<Self>, | ||
| __location_stack: &mut Vec<#location_typename>, | ||
| ) -> #token_data_typename { | ||
| let ret = __data_stack.swap_remove( __data_stack.len() - #tokens_len + #identity_token_idx ); |
There was a problem hiding this comment.
Using swap_remove here will move the last element to the removed position, potentially corrupting the stack order. Use regular indexing or remove() instead to maintain proper stack semantics.
| let ret = __data_stack.swap_remove( __data_stack.len() - #tokens_len + #identity_token_idx ); | |
| let ret = __data_stack.remove( __data_stack.len() - #tokens_len + #identity_token_idx ); |
nondeterministic::Context::prepare_reduce_node()to remove reverse order stack copying