Skip to content
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.

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

Fix interpet's use of stale memory #534

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Conversation

krtab
Copy link
Collaborator

@krtab krtab commented Mar 3, 2025

Each time the memory is cloned, previous versions should be considered stale. This is especially true for #433 where the inner data structures are made purely functional, but is likely to cause bugs even without.

We should think of better ways to ensure this invariant is not broken, as the bugs caused are pretty nasty to track down.

Each time the memory is cloned, previous versions should be considered
stale. This is especially true for OCamlPro#433
where the inner data structures are made purely functional, but is
likely to cause bugs even without.
@krtab krtab requested a review from zapashcanon March 3, 2025 17:18
@zapashcanon
Copy link
Member

There are three cases:

  • we have to add/move the let* mem after cloning, this is needed to be correct;
  • we simply move down the let* mem even without cloning, this is more future proof;
  • we add a new let* mem even when there's no cloning, this is more future proof but it also makes things slower.

I'm fine about keeping things as is but I also think we could remove the last case?

I'll let you choose what you prefer and then I'll merge.

@zapashcanon zapashcanon merged commit b4cf785 into OCamlPro:main Mar 3, 2025
4 of 5 checks passed
@zapashcanon
Copy link
Member

Fine after discussing!

(The `let> is actually hidding a select)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants