Skip to content

Conversation

@yannham
Copy link
Collaborator

@yannham yannham commented Jul 25, 2025

Yet another slice of work on the compact value representation, split off from #2302. This PR:

  1. Moves position-related stuff to the position module instead, to avoid bloating value too much
  2. Adds a way to mutably access the content of a value block, whenever possible. This is something that is possible with the current term representation, and we'll need to match this functionality.

In general, additions to the value module are driven by the needs encountered when working on #2302.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2025

@yannham yannham marked this pull request as ready for review July 25, 2025 11:10
@yannham yannham requested a review from jneem July 25, 2025 11:10
@yannham yannham force-pushed the rfc007/value-lens branch from 43e92b1 to 8051133 Compare July 25, 2025 11:31
@yannham yannham force-pushed the rfc007/value-lens branch from 8051133 to 47ef78e Compare July 25, 2025 11:35
// over `self`, so there can't be other active mutable borrows to the block
Some(match header.tag {
BodyTag::Number => ValueContentRefMut::Number(
ValueBlockRc::decode_mut_from_raw_unchecked(as_ptr),
Copy link
Member

Choose a reason for hiding this comment

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

Does it work to let rc = ValueBlockRc::decode_mut_from_raw_unchecked(as_ptr), so you don't have to repeat the part? Or is there some subtlety that I'm missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

decode_xxx takes a type parameter indicating to which type this function should decode. Here it's hidden because the type checker has enough information to deduce what is this target type T (given the definition of ValueContentRefMut::XXX), but each of this calls return a totally different type, so we can't really factor them out.

/// increments the reference count but encapsulate a pointer to the same block in memory (as
/// [std::rc::Rc::clone], this method actually allocates a fresh block with a copy of the
/// content and return a value that is 1-reference counted.
pub fn deep_copy(&self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I usually think "deep copy" means that it also descends recursively and deep copies all references (e.g. https://docs.python.org/3/library/copy.html).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair, I'll try to come up with a better name.

@yannham yannham added this pull request to the merge queue Jul 31, 2025
Merged via the queue into master with commit 5394409 Jul 31, 2025
5 checks passed
@yannham yannham deleted the rfc007/value-lens branch July 31, 2025 10:20
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.

3 participants