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

memory.c doc: add a note on the 'mixed' memory non-model #12473

Merged
merged 1 commit into from Aug 6, 2023

Conversation

gasche
Copy link
Member

@gasche gasche commented Aug 6, 2023

This PR takes a description of our current understanding of how to write C code for the OCaml runtime, written jointly with @gadmm in #10992 (comment) , and includes it as a comment in memory.c, as suggested by @OlivierNicole. The purpose is to store our collective knowledge about this tricky issue in a place that is more likely to be found in the future than a comment in an github issue.

(cc also @xavierleroy, @kayceesrk, @jhjourdan who may have opinions about this)

The original message contains a description of "future work" to improve the situation / our current understanding, which I find interesting but did not include in the documentation comment.

How to review?

Please let me know if:

  1. you strongly disagree with having such content as a comment in the codebase
  2. you disagree with something written in the comment

If neither (1) or (2) hold, then you may want to approve :-)

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

I'm not sure why I was summoned, but I find this documentation useful, esp. the link to "the Linux memory model", which I hadn't read before. We can always argue what's the best place for such documentation: personally, I believe that comments in an implementation file (.c file) should explain the implementation, not the interface nor the high-level design as in the case here. But there is precedent (the [MM] comment in this file), and it's better than nothing, and it can be changed later if someone bothers to collect all these memory model comments in one place.

Co-authored-by: Guillaume Munch-Maccagnoni <Guillaume.Munch-Maccagnoni@inria.fr>
Reported-by: Olivier Nicole <olivier@chnik.fr>
@gasche
Copy link
Member Author

gasche commented Aug 6, 2023

I'm not sure why I was summoned, [provides useful feedback and lets the PR make progress towards merging].

Thanks!

@gasche gasche added the merge-me label Aug 6, 2023
@gasche gasche merged commit 3e2f3a8 into ocaml:trunk Aug 6, 2023
10 checks passed
@gadmm
Copy link
Contributor

gadmm commented Aug 10, 2023

Thanks @gasche

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

Successfully merging this pull request may close these issues.

None yet

3 participants