Skip to content

Fix assertion in caml_ephe_clean#12860

Merged
Octachron merged 1 commit intoocaml:trunkfrom
mshinwell:fix-is-marked-assertion
Feb 21, 2024
Merged

Fix assertion in caml_ephe_clean#12860
Octachron merged 1 commit intoocaml:trunkfrom
mshinwell:fix-is-marked-assertion

Conversation

@mshinwell
Copy link
Copy Markdown
Contributor

This assertion wasn't taking into account the possibility of an ephemeron pointing at static data.
(Will add Changes entry later.)

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

This is correct.

(There is no other use of is_marked anywhere, so I wondered if the definition instead of the callsite should be updated, possibly with a rename to is_marked_or_unmarkable. But I have no strong opinion and @mshinwell knows this code better.)

@mshinwell
Copy link
Copy Markdown
Contributor Author

@kayceesrk do you have an opinion on this point?

Copy link
Copy Markdown
Contributor

@kayceesrk kayceesrk left a comment

Choose a reason for hiding this comment

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

This change looks fine to me.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jan 5, 2024

Please add a Changes entry for 5.2.

@Octachron Octachron self-assigned this Feb 21, 2024
@Octachron Octachron merged commit 5cfc104 into ocaml:trunk Feb 21, 2024
Octachron added a commit that referenced this pull request Feb 21, 2024
Fix assertion in caml_ephe_clean

(cherry picked from commit 5cfc104)
@Octachron
Copy link
Copy Markdown
Member

Changes entry added and PR cherry-picked to 5.2.

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.

4 participants