check if custom reduce action can be identity #31
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements an optimization to detect when custom reduce actions can be treated as identity operations (returning a single token unchanged). The main changes enhance the parser generator to analyze custom reduce actions and convert simple cases like { A } to more efficient identity operations.
- Introduces intelligent detection of identity reduce actions in custom code
- Refactors
ReduceAction::Customto use a newCustomReduceActionwrapper that tracks identifier usage - Optimizes generated parser code by simplifying identity operations and removing unused function parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rusty_lr_parser/src/nonterminal_info.rs | Adds CustomReduceAction struct to track identifier usage in custom reduce actions |
| rusty_lr_parser/src/grammar.rs | Implements logic to detect when custom reduce actions are equivalent to identity operations |
| rusty_lr_parser/src/emit.rs | Updates code generation to use the new CustomReduceAction interface and optimizes parameter usage |
| rusty_lr_parser/src/pattern.rs | Updates all ReduceAction::Custom constructors to use new_custom method |
| scripts/diff/calculator.rs | Shows generated code changes with optimized reduce function signatures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
scripts/diff/calculator.rs
Outdated
| // | ||
| // ================================User Codes Begin================================ | ||
| #[derive(Debug, Clone, Copy)] | ||
| #[derive(Debug, Clone, Copy)]a |
There was a problem hiding this comment.
There's an extra 'a' character at the end of the derive attribute line that appears to be a typo.
| #[derive(Debug, Clone, Copy)]a | |
| #[derive(Debug, Clone, Copy)] |
Copilot uses AI. Check for mistakes.
| pub struct CustomReduceAction { | ||
| pub body: TokenStream, | ||
| idents_used: BTreeSet<Ident>, | ||
| } |
There was a problem hiding this comment.
[nitpick] The idents_used field should be public or provide a getter method since it's used for optimization decisions. Currently it's private but accessed through contains_ident method, which is good design.
Copilot uses AI. Check for mistakes.
| Some(ReduceAction::Custom(new_reduce_action)) | ||
|
|
||
| // check if this reduce action can be identity action; i.e., { $1 } | ||
| fn tokenstream_contains_unique_ident(ts: TokenStream) -> Option<Ident> { |
There was a problem hiding this comment.
[nitpick] This nested function should be extracted as a module-level function or method to improve readability and testability of this complex logic.
Copilot uses AI. Check for mistakes.
No description provided.