Skip to content

use truncate() instead of pop() for unused and shadowed variables#23

Merged
ehwan merged 6 commits into
mainfrom
location_unused
Aug 11, 2025
Merged

use truncate() instead of pop() for unused and shadowed variables#23
ehwan merged 6 commits into
mainfrom
location_unused

Conversation

@ehwan

@ehwan ehwan commented Aug 11, 2025

Copy link
Copy Markdown
Owner

No description provided.

@ehwan
ehwan requested a review from Copilot August 11, 2025 15:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes the parser code generation by replacing individual pop() calls with more efficient truncate() operations for unused and shadowed variables on parser stacks. This reduces unnecessary work when variables are not referenced in the reduce action code.

Key changes:

  • Implements analysis to detect unused location variables in reduce actions
  • Replaces multiple consecutive pop() operations with single truncate() calls
  • Handles variable shadowing by nullifying earlier declarations of the same variable name

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
rusty_lr_parser/src/parser/parser_expanded.rs Generated parser code updated to use truncate() instead of pop() for unused variables
rusty_lr_parser/src/emit.rs Code generation logic enhanced to analyze variable usage and emit optimized stack operations
SYNTAX.md Documentation updated to clarify variable access patterns in reduce actions

Comment on lines 1263 to 1281
for (token_index_from_end, token) in rule.tokens.iter().rev().enumerate() {
let stack_name = token_to_stack_name(token.token);
let tag_name = stack_name.unwrap_or(&empty_tag_name);

#[derive(PartialEq, Eq, PartialOrd, Ord)]
enum StackName {
DataStack(Ident),
LocationStack,
}
impl StackName {
pub fn to_token_stream(&self) -> TokenStream {
match self {
StackName::DataStack(name) => quote! {__data_stack.#name},
StackName::LocationStack => quote! {__location_stack},
}
}
}

if let Some(stack_name) = stack_name {

Copilot AI Aug 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StackName enum definition is placed inside the loop, which means it gets redefined for every iteration. This should be moved outside the loop to avoid redundant type definitions.

Suggested change
for (token_index_from_end, token) in rule.tokens.iter().rev().enumerate() {
let stack_name = token_to_stack_name(token.token);
let tag_name = stack_name.unwrap_or(&empty_tag_name);
#[derive(PartialEq, Eq, PartialOrd, Ord)]
enum StackName {
DataStack(Ident),
LocationStack,
}
impl StackName {
pub fn to_token_stream(&self) -> TokenStream {
match self {
StackName::DataStack(name) => quote! {__data_stack.#name},
StackName::LocationStack => quote! {__location_stack},
}
}
}
if let Some(stack_name) = stack_name {
#[derive(PartialEq, Eq, PartialOrd, Ord)]
enum StackName {
DataStack(Ident),
LocationStack,
}
impl StackName {
pub fn to_token_stream(&self) -> TokenStream {
match self {
StackName::DataStack(name) => quote! {__data_stack.#name},
StackName::LocationStack => quote! {__location_stack},
}
}
}
for (token_index_from_end, token) in rule.tokens.iter().rev().enumerate() {
let stack_name = token_to_stack_name(token.token);
let tag_name = stack_name.unwrap_or(&empty_tag_name);

Copilot uses AI. Check for mistakes.
Comment on lines +1288 to +1307
fn tokenstream_contains_ident(
stream: TokenStream,
ident: &Ident,
) -> bool {
for t in stream {
match t {
proc_macro2::TokenTree::Ident(i) if &i == ident => {
return true
}
proc_macro2::TokenTree::Group(g) => {
if tokenstream_contains_ident(g.stream(), ident) {
return true;
}
}
_ => {}
}
}
false
}

Copilot AI Aug 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tokenstream_contains_ident function is defined inside the loop, causing it to be redefined on every iteration. This helper function should be moved outside the loop or to module level for better performance and code organization.

Suggested change
fn tokenstream_contains_ident(
stream: TokenStream,
ident: &Ident,
) -> bool {
for t in stream {
match t {
proc_macro2::TokenTree::Ident(i) if &i == ident => {
return true
}
proc_macro2::TokenTree::Group(g) => {
if tokenstream_contains_ident(g.stream(), ident) {
return true;
}
}
_ => {}
}
}
false
}

Copilot uses AI. Check for mistakes.
@ehwan
ehwan marked this pull request as ready for review August 11, 2025 15:10
@ehwan
ehwan merged commit 74fe9c7 into main Aug 11, 2025
1 check passed
@ehwan
ehwan deleted the location_unused branch August 11, 2025 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants