Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce backwards compatible infrastructure for parallelism #1708
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
base: master
Are you sure you want to change the base?
Introduce backwards compatible infrastructure for parallelism #1708
Changes from 17 commits
b1d786f
1667683
f9ed034
bc33346
67a1c61
3b9d901
539a853
3f7d226
48c76c0
7fe63ef
490d047
16db900
dec26b3
f423d8e
ccb2a6f
a18cd77
9e1614d
92043ca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this about? Isn't it just a copy of what's in
Stdlib.Format
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module now uses
BatFormat
, which does not define the method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why
BatFormat
? The standard library version from OCaml 5 should be safe: it's been reimplemented to use DLS properly. Batteries as no domains-specific fixes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're taking the mutex implementation from domain-local-await, why not the lazy? https://github.com/ocaml-multicore/domain-local-await?tab=readme-ov-file#example-concurrency-safe-lazy (pending the licence question below)
In particular, that one seems to properly handle exceptions, whereas the one here would royally break: the mutex wouldn't be unlocked even and any future attempts to lock it will block forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is batteries used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to have
.mli
files for these as well (not dependent on domainlib presence).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big question with this is whether this licence just allows us to swallow the code into MIT-licenced Goblint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ther ISC license is compatible in spirit with the MIT license. Maybe we just leave the license notice in this file as is and add at the top in the LICENSE.md that the MIT license applies to all files unless specified otherwise?
I think polluting the opam repo with a package containing only this file for trivial license stuff may be excessive.