Skip to content

fix: wrap FieldPath inner Vec in Rc for O(1) cloning#84

Closed
eikopf wants to merge 1 commit intomainfrom
fix/32-fieldpath-rc-clone
Closed

fix: wrap FieldPath inner Vec in Rc for O(1) cloning#84
eikopf wants to merge 1 commit intomainfrom
fix/32-fieldpath-rc-clone

Conversation

@eikopf
Copy link
Copy Markdown
Owner

@eikopf eikopf commented Mar 14, 2026

Summary

  • Wraps FieldPath's inner Vec<PathSegment> in Rc so that clone() is O(1) instead of O(n)
  • Makes the inner field private, adding a segments() accessor for read access
  • Pre-allocates exact capacity in push() instead of clone + push
  • Updates two call sites that accessed the inner .0 field directly (eval.rs test and render.rs display impl)

Test plan

  • All 566 existing workspace tests pass (cargo test --workspace)
  • No public API changes beyond making the inner field private (was only accessed in two internal locations)

Closes #32

🤖 Generated with Claude Code

FieldPath values are frequently cloned when stored in Blame objects
during record lowering. By wrapping the inner Vec<PathSegment> in Rc,
clones become O(1) reference count bumps instead of O(n) vector copies.

The push method now pre-allocates with exact capacity instead of
clone + push, and the inner field is made private with a segments()
accessor to maintain encapsulation.

Closes #32

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@eikopf
Copy link
Copy Markdown
Owner Author

eikopf commented Mar 14, 2026

It's not clear to me that issue #32 is actually correct? I suspect it's being properly interned by salsa, and if that's not the case then we should address this by interning the path rather than wrapping it with Rc.

@eikopf
Copy link
Copy Markdown
Owner Author

eikopf commented Mar 14, 2026

Superseded by #91

@eikopf eikopf closed this Mar 14, 2026
@eikopf eikopf deleted the fix/32-fieldpath-rc-clone branch March 14, 2026 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.

FieldPath::push clones the entire vector on every call

1 participant