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

Introduce extended specs to allow wrapping command sequences #511

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

polytypic
Copy link
Contributor

@polytypic polytypic commented Jan 14, 2025

See #510.

Contrary to what #510 concluded with, I decided to introduce

  • an extended Spec signature, SpecExt,
  • a module of default specs, SpecDefaults,
  • and extended functors, STM_sequential.MakeExt, STM_domain.MakeExt, and STM_thread.MakeExt.

The reason for this is that introducing wrapped variants of the various functions produced by the functors would have roughly meant duplicating all of the functions. This approach is much more incremental and also potentially allows the Spec interface to the extended in the future.

The idea is that one always uses the SpecDefaults:

module MySpec = struct
  include SpecDefaults
  ...
end

This way new optional specs can be added as long as SpecDetaults can provide default implementations of such additions.

There is a PR to Picos that uses this PR.

TODO:

  • Agree this is the way to go
  • Document the API additions
  • Finish the implementation, including creating STM_thread.MakeExt, and making sure operations are wrapped in all cases
  • Update changelog

@polytypic polytypic linked an issue Jan 14, 2025 that may be closed by this pull request
@polytypic polytypic force-pushed the introduce-extended-specs-to-allow-wrapping branch from 19a43c2 to b4a8b0a Compare January 14, 2025 10:15
@polytypic polytypic force-pushed the introduce-extended-specs-to-allow-wrapping branch 3 times, most recently from 413df00 to 97462a8 Compare January 14, 2025 15:30
@jmid
Copy link
Collaborator

jmid commented Jan 14, 2025

Thanks for this! 🙏

I've played a bit with it locally, rewriting existing STM tests to use it. This made me realize that both MakeExt and Make clients can use include SpecDefaults which is a net-win IMO - even for users just sticking to Make.
To ensure against regressions, I've also checked that this shouldn't cause error finding regressions, by rerunning the statistical tests.

I spotted locally that this would emit a compiler warning, but I believe you've fixed that in a recent (force) push.

Naming-wise I think

  • SpecExt and MakeExt are reasonable for extended specs
  • SpecDefaults is also fair - and also works well with the STM. quantification

My only reservation is about the wrap name. I don't think it describes well what is being wrapped.
Something like wrap_cmd_seq or wrap_cmds would be a bit more saying. I'm open to other suggestions - and not particularly eager about any of these two. The doc-string for (the binding-formerly-known-as) wrap should probably also describe a bit more what is being wrapped.

Otherwise, I'm OK with this 🙂 👍

@polytypic polytypic force-pushed the introduce-extended-specs-to-allow-wrapping branch from 97462a8 to 25cba4a Compare January 14, 2025 16:49
@polytypic
Copy link
Contributor Author

polytypic commented Jan 14, 2025

I decided to rename to wrap_cmd_seq, because wrap_cmds might give the impression of wrapping each and every command separately.

@polytypic polytypic changed the title Introduce extended specs to allow wrapping Introduce extended specs to allow wrapping command sequences Jan 14, 2025
@polytypic polytypic force-pushed the introduce-extended-specs-to-allow-wrapping branch from 25cba4a to 0742dc4 Compare January 14, 2025 17:01
@polytypic polytypic marked this pull request as ready for review January 14, 2025 17:38
The motivating use case is to make it nicer to test commands that perform
effects.
@polytypic polytypic force-pushed the introduce-extended-specs-to-allow-wrapping branch from 0742dc4 to 45ae1ad Compare January 14, 2025 21:03
lib/STM.mli Outdated Show resolved Hide resolved
lib/STM.mli Outdated Show resolved Hide resolved
Copy link
Collaborator

@jmid jmid left a comment

Choose a reason for hiding this comment

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

LGTM. I spotted a couple of minor documentation nits while proof-reading.

polytypic and others added 2 commits January 14, 2025 22:49
Co-authored-by: Jan Midtgaard <[email protected]>
Co-authored-by: Jan Midtgaard <[email protected]>
@jmid
Copy link
Collaborator

jmid commented Jan 15, 2025

CI summary for 263ae96: all 34 workflows passed.
Merging...

@jmid jmid merged commit 59296ec into main Jan 15, 2025
34 checks passed
@jmid
Copy link
Collaborator

jmid commented Jan 15, 2025

CI summary for merge to main:

Out of 35 workflows - 1 failed with a CI related error

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.

Support testing effects based multicore primitives with QCheck STM
2 participants