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

Add cstubs_applicative library #5847

Merged
merged 5 commits into from
Sep 4, 2020
Merged

Add cstubs_applicative library #5847

merged 5 commits into from
Sep 4, 2020

Conversation

mrmr1993
Copy link
Member

@mrmr1993 mrmr1993 commented Sep 3, 2020

This PR creates the cstubs_applicative library. The purpose of this is to allow us to bind or otherwise use cstubs bindings at their definition points, instead of needing a complex system with multiple layers. Intended uses are:

  • ensuring that delete is always called upon garbage collection of foreign values
    • we currently have memory leaks that result from this
  • allow OCaml types to be constructed directly at the bindings point from foreign record/struct types in a memory efficient way
  • avoid duplication between the base interface and the OCaml-friendly interface.

For example:

let delete = foreign (prefix "delete") (typ @-> void)
let create = map2 (foreign (prefix "create") (A.typ @-> B.typ @-> typ)) delete ~f:(fun f delete ->
  let schedule_delete = Caml.Gc.finalise delete in
  fun ~a ~b ->
    let ret = f a b in
    schedule_delete ret ;
    ret

Checklist:

  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

@mrmr1993 mrmr1993 requested a review from a team as a code owner September 3, 2020 19:10
@@ -0,0 +1,22 @@
opam-version: "2"
Copy link
Member

Choose a reason for hiding this comment

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

Our other .opam files should be this nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I plan to use this in the zexe submodule, so it needed to be opam pin-able for standalone use there, and it only seemed right to do it properly!

Copy link
Member

@imeckler imeckler left a comment

Choose a reason for hiding this comment

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

Neat

@mrmr1993 mrmr1993 added ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge-into-develop labels Sep 4, 2020
@mergify mergify bot merged commit dda5127 into develop Sep 4, 2020
@mergify mergify bot deleted the feature/ctypes-applicative branch September 4, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge-into-develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants