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 an OCaml<DynBox> type for passing Rust values through OCaml #24

Closed
wants to merge 4 commits into from

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Feb 19, 2021

There's also a few commits that help with testing:

  • expose cons for building lists incrementally
  • expose Gc.compact to tests
  • expose the pointer on OCaml custom types

@g2p
Copy link
Contributor Author

g2p commented Feb 19, 2021

Note: I broke down the commits like this so I could share some parts with other PRs (like the one for bigarrays).

That said, while it's fairly granular for reviewing, I can also rebase it to make a big PR if you don't want merge conflicts (mostly imports and tests).

@tizoc
Copy link
Owner

tizoc commented Feb 19, 2021

@g2p thank you! I can't go through this one today, but on the surface it looks good. I will do a more thorough review once I'm free from other stuff

@g2p
Copy link
Contributor Author

g2p commented Mar 5, 2021

@tizoc Hello! I hope you can find time for reviewing these

@tizoc
Copy link
Owner

tizoc commented Mar 5, 2021

@g2p yes, and sorry! I have not forgotten, the main reason I haven't yet is that I'm waiting to integrate BoxRoots first, which will impact everything in ocaml-interop (and anything new that comes). I planned on doing that weeks ago but some urgent things got in the way, but will be taking care of it very soon.

@tizoc
Copy link
Owner

tizoc commented Mar 15, 2021

One thing that I would change here is the OCamlBox name, because BoxRoot already exists, and could be confusing.

The other thing I was thinking: how far is this from a full custom blocks implementation? Thinking of https://github.com/zshipko/ocaml-rs/blob/master/src/custom.rs

I think it has a good API. Would it make more sense to have that ported?

g2p added 3 commits April 14, 2021 14:38
Useful for bigarrays and other custom types.
Building lists incrementally allows more control over conversion.
@g2p
Copy link
Contributor Author

g2p commented Apr 16, 2021

I've rebased this (and the rest) after the introduction of BoxRoots.

Re the name: yeah, it was meant to reference Rust's Box as a generic way of moving Rust data into allocated memory. Not sure what to use instead. OCaml<DynBox<T>> would also work and be fairly accurate, does that still risk confusion? OCaml<RustBox<T>> maybe, to contrast with BoxRoots containing OCaml data? OCaml<GenWrapper<T>>?

Re generic custom blocks: the rest of the vtable is for serialisation and generic comparisons. Mapping the whole generic functionality of the C API is meaningful for ocaml-sys, this OCamlBox (or whatever the new name is) is just one kind of custom block with one vtable function, but one that works well for wrapping arbitrary data.

I could see it being specialized to skip the dyn on Rust's side, and maybe put the inner T data directly in the OCaml block (assuming Unpin and proper alignment), but I don't see something as generic as the C API as adding value over the existing low-level abstraction.

@tizoc
Copy link
Owner

tizoc commented Apr 16, 2021

OCaml<DynBox<T>> would also work and be fairly accurate, does that still risk confusion?

No, I think that one is quite good (DynBox, RustValue, etc, not sure what is best, but I like the overall concept).

I'm actually doing something very similar already in code that uses custom blocks and ocaml-interop, just have not made it part of ocaml-interop because I am not entirely sure about what the API should look like and the code I have now is very ugly and written in haste (you can see it here).

Re generic custom blocks: the rest of the vtable is for serialisation and generic comparisons. Mapping the whole generic functionality of the C API is meaningful for ocaml-sys, this OCamlBox (or whatever the new name is) is just one kind of custom block with one vtable function, but one that works well for wrapping arbitrary data.

Well, for now none of the other things are used, but I don't think support for that is useless. Doesn't mean that it has to be done now, but it would be good to keep it in mind and leave space for implementing it in the future.

@g2p
Copy link
Contributor Author

g2p commented Apr 16, 2021

Well, for now none of the other things are used, but I don't think support for that is useless. Doesn't mean that it has to be done now, but it would be good to keep it in mind and leave space for implementing it in the future.

Okay, agreed. For example, a macro that would register user-defined custom types by TypeId so that we could add more functionality if necessary, and cast back safely.

It doesn't necessarily need to be in this PR, assuming OCaml<DynBox> is sufficiently useful on its own.

This uses a Pin<Box<dyn Any>> internally, for safe downcasting and
proper dropping when OCaml finalizes the custom type.
@g2p
Copy link
Contributor Author

g2p commented Apr 16, 2021

(Pushed again with the new name)

@g2p g2p changed the title Add an OCamlBox type for passing Rust values through OCaml Add an OCaml<DynBox> type for passing Rust values through OCaml Apr 16, 2021
src/memory.rs Show resolved Hide resolved
src/memory.rs Show resolved Hide resolved
}
}

/// Implementation notes: is it possible to reduce indirection?
Copy link
Owner

Choose a reason for hiding this comment

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

@g2p can you rebase this one against master (merging #20 caused a conflict) and change this comment so that it is not a documentation comment? (what is said here is more about internal implementation details)

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, nevermind, I am doing it on a new branch where I'm making a few changes and updating the CHANGELOG before merging.

@tizoc tizoc mentioned this pull request Apr 24, 2021
@tizoc
Copy link
Owner

tizoc commented Apr 24, 2021

Closing this one, will be merged in #31.

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.

3 participants