add IntermediateState for lightweight state generation#22
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new IntermediateState struct to serve as a lightweight intermediate representation for state generation in the LR parser. The change refactors the code generation and state conversion pipeline to use Vec instead of BTreeMap/BTreeSet collections for better performance during code generation.
Key changes:
- Introduces
IntermediateStatestruct withVec-based fields for shift/goto maps and reduce rules - Replaces
BTreeMap/BTreeSetusage withVecin code generation - Updates conversion logic to handle sorted vector extraction instead of map key removal
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| rusty_lr_parser/src/emit.rs | Updates code generation to use IntermediateState and Vec collections instead of BTreeMap/BTreeSet |
| rusty_lr_core/src/parser/state.rs | Adds IntermediateState struct and updates conversion logic to handle vector-based state representations |
| rusty_lr_core/src/builder/state.rs | Adds conversion from builder State to IntermediateState |
Comments suppressed due to low confidence (1)
| /// this intermediate_state is common structure to convert from generated code, grammar builder | ||
| /// into various types of parser states ( SparseState, DenseState, ... ) | ||
| pub struct IntermediateState<Term, NonTerm, StateIndex, RuleIndex> { | ||
| pub shift_goto_map_term: Vec<(Term, StateIndex)>, // must be sorted |
There was a problem hiding this comment.
The sorting requirement is critical for correctness but only mentioned in comments. Consider adding runtime assertions or using a type that enforces this invariant.
| debug_assert!( | ||
| TerminalSymbol::Term(0) < TerminalSymbol::Error | ||
| && TerminalSymbol::<i32>::Error < TerminalSymbol::Eof | ||
| ); |
There was a problem hiding this comment.
The hardcoded TerminalSymbol::Term(0) in the assertion makes assumptions about the type parameter. Consider using a more generic approach or documenting this constraint.
| debug_assert!( | |
| TerminalSymbol::Term(0) < TerminalSymbol::Error | |
| && TerminalSymbol::<i32>::Error < TerminalSymbol::Eof | |
| ); | |
| // Removed hardcoded assertion about TerminalSymbol ordering to avoid assumptions about type parameter. |
| debug_assert!( | ||
| TerminalSymbol::Term(0) < TerminalSymbol::Error | ||
| && TerminalSymbol::<i32>::Error < TerminalSymbol::Eof | ||
| ); | ||
| let keys = builder_state | ||
| .shift_goto_map_term | ||
| .iter() | ||
| .map(|(term, _)| term) | ||
| .collect::<Vec<_>>(); | ||
| debug_assert!(keys.is_sorted()); | ||
|
|
||
| let keys = builder_state | ||
| .reduce_map | ||
| .iter() | ||
| .map(|(term, _)| term) | ||
| .collect::<Vec<_>>(); | ||
| debug_assert!(keys.is_sorted()); |
There was a problem hiding this comment.
Duplicate assertion logic exists in both SparseState and DenseState conversion methods. Consider extracting this into a shared validation function.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
this could reduce binary size by 10%
we were using
builder::Statefor state generation which was usingBTreeMap.add new
IntermediateStatestruct for common type convertion into any parser state type, and it is usingVec.