performance(strings): Using less interim strings. - #10126
Conversation
|
This change is |
PR SummaryLow Risk Overview Touches lowering ( Reviewed by Cursor Bugbot for commit 3d9e6b3. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 7 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-sierra-generator/src/program_generator.rs line 256 at r1 (raw file):
f, "{}", loc.split('\n').format_with("\n", |l, out| out(&format_args!("// {l}")))
Why out? better to rename f to keep the format_with style the same IMO
9bba9d0 to
8914649
Compare
76f2fca to
3d9e6b3
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-sierra-generator/src/program_generator.rs line 256 at r1 (raw file):
Previously, eytan-starkware wrote…
Why out? better to rename f to keep the format_with style the same IMO
i think the AI decided it was confusing since there's another f here.
but Done.
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware).
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on orizi).
Summary
Replaces
.map(|x| format!(...)).format(sep)and.map(|x| format!(...)).join(sep)patterns with.format_with(sep, |x, f| f(&format_args!(...)))across several crates. This avoids allocating an intermediateStringfor each element during formatting.Type of change
Please check one:
Why is this change needed?
The previous pattern called
.map(|x| format!(...))to produce aStringper element before joining or formatting them together.itertools::format_withallows formatting each element directly into the output buffer viaformat_args!, eliminating the per-element heap allocation.What was the behavior or documentation before?
Each formatted element was first heap-allocated as a
String, then those strings were joined or formatted into the final output string.What is the behavior or documentation after?
Each element is formatted directly into the output using
format_args!, avoiding intermediateStringallocations per element.Related issue or discussion (if any)
None.
Additional context
The affected sites are all in diagnostic/debug formatting paths (
DiagnosticEntry,Debugimpls, assertion messages, and test output), so correctness is unchanged — only allocation behavior differs.