-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
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). |
@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 |
@tizoc Hello! I hope you can find time for reviewing these |
@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. |
One thing that I would change here is the 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? |
Useful for bigarrays and other custom types.
Building lists incrementally allows more control over conversion.
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. 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. |
No, I think that one is quite good ( I'm actually doing something very similar already in code that uses custom blocks and
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.
(Pushed again with the new name) |
} | ||
} | ||
|
||
/// Implementation notes: is it possible to reduce indirection? |
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.
Actually, nevermind, I am doing it on a new branch where I'm making a few changes and updating the CHANGELOG before merging.
Closing this one, will be merged in #31. |
There's also a few commits that help with testing: