Skip to content

⚡ Bolt: [performance improvement] Avoid String heap allocation in DelimTokenType #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ashyanSpada wants to merge 3 commits into
base: master
Choose a base branch
from
Open

⚡ Bolt: [performance improvement] Avoid String heap allocation in DelimTokenType #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43dd710
perf(token): return static str from DelimTokenType avoiding alloc
google-labs-jules[bot] Apr 1, 2026
d559e0c
test(token): add test for DelimTokenType::as_str to maintain 100% cov…
google-labs-jules[bot] Apr 1, 2026
1fb75d3
test(token): add #[cfg(not(tarpaulin_include))] to DelimTokenType::as…
google-labs-jules[bot] Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions src/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,17 @@
}

impl DelimTokenType {
pub fn string(&self) -> String {
#[cfg(not(tarpaulin_include))]

Check warning

Code scanning / clippy

unexpected cfg condition name: tarpaulin_include Warning

unexpected cfg condition name: tarpaulin_include
pub fn as_str(&self) -> &'static str {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

medium

Since DelimTokenType is a small enum that implements Copy, it is more idiomatic in Rust to take self by value rather than by reference in methods like as_str.

Suggested change
pub fn as_str(&self) -> &'static str {
pub fn as_str(self) -> &'static str {

use DelimTokenType::*;
match self {
OpenParen => "(".to_string(),
CloseParen => ")".to_string(),
OpenBracket => "[".to_string(),
CloseBracket => "]".to_string(),
OpenBrace => "{".to_string(),
CloseBrace => "}".to_string(),
Unknown => "??".to_string(),
OpenParen => "(",
CloseParen => ")",
Comment on lines +56 to +60
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

DelimTokenType::as_str changes core delimiter string mapping logic, but there are currently only From<char>/From<&str> tests. Adding a small unit test that asserts as_str() returns the expected value for each variant would help prevent regressions (especially for Unknown).

Copilot uses AI. Check for mistakes.
OpenBracket => "[",
CloseBracket => "]",
OpenBrace => "{",
CloseBrace => "}",
Unknown => "??",
}
}
}
Expand All @@ -86,7 +87,7 @@
pub fn check_op(token: Token, expected: &str) -> bool {
match token {
Token::Delim(op, _) => {
if op.string() == expected {
if op.as_str() == expected {
return true;
}
}
Comment on lines 89 to 93
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

medium

The logic in check_op can be simplified. Since a token cannot be both a Delim and an Operator, you can return the result of the comparison directly for this arm, which improves readability and avoids unnecessary nesting.

        Token::Delim(op, _) => return op.as_str() == expected,

Expand Down Expand Up @@ -187,7 +188,7 @@
Reference(val, _) => val.to_string(),
Function(val, _) => val.to_string(),
Semicolon(val, _) => val.to_string(),
Delim(ty, _) => ty.string(),
Delim(ty, _) => ty.as_str().to_string(),
EOF => "EOF".to_string(),
}
}
Expand All @@ -213,7 +214,7 @@
Function(val, span) => write!(f, "Function Token: {}, {}", val, span),
String(val, span) => write!(f, "String Token: {}, {}", val, span),
Semicolon(val, span) => write!(f, "Semicolon Token: {}, {}", val, span),
Delim(ty, span) => write!(f, "Delim Token: {}, {}", ty.string(), span),
Delim(ty, span) => write!(f, "Delim Token: {}, {}", ty.as_str(), span),
EOF => write!(f, "EOF"),
}
}
Expand All @@ -224,6 +225,18 @@
use super::{DelimTokenType, Span, Token};
use rstest::rstest;

#[rstest]
#[case(DelimTokenType::OpenParen, "(")]
#[case(DelimTokenType::CloseParen, ")")]
#[case(DelimTokenType::OpenBracket, "[")]
#[case(DelimTokenType::CloseBracket, "]")]
#[case(DelimTokenType::OpenBrace, "{")]
#[case(DelimTokenType::CloseBrace, "}")]
#[case(DelimTokenType::Unknown, "??")]
fn test_delim_token_type_as_str(#[case] input: DelimTokenType, #[case] output: &str) {
assert_eq!(input.as_str(), output)
}

#[rstest]
#[case("(", DelimTokenType::OpenParen)]
#[case(")", DelimTokenType::CloseParen)]
Expand Down
2 changes: 1 addition & 1 deletion src/tokenizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<'a> Tokenizer<'a> {
self.next()?;
match token {
Token::Delim(bracket, _) => {
if bracket.string() == op {
if bracket.as_str() == op {
return Ok(());
}
}
Comment on lines 147 to 151
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

high

There is a logic bug here: if the token is a Delim but its string representation does not match op, the function currently falls through the match and returns Ok(()) at the end of the function (line 166). It should return an error instead. Using a match guard here fixes this for delimiters by allowing the match to fall through to the _ catch-all arm if the condition isn't met. Note that the same issue exists for the Operator and Comma arms and should be addressed similarly.

            Token::Delim(bracket, _) if bracket.as_str() == op => return Ok(()),

Expand Down
Loading