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

allow to_ocaml to take owned self #55

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tiny-book2
Copy link

This is a rough proposal (with code!) to update ToOCaml::to_ocaml to take self instead of &self. It also changes many default implementations of ToOCaml to be implemented on &SomeType instead of SomeType. As far as I understand it:

  • this usually won't affect the ergonomics of calling to_ocaml, since Rust will automatically take a reference of when calling a method on it.
  • if you're implementing a generic function, and want to make sure you can convert just a reference of something into OCaml without giving up ownership, you can still add the type constraint for <'a> &'a T: ToOCaml
  • i think this allows users to implement more efficient conversions in some cases --- for example, perhaps an owned String could be converted into a Bigarray without copying?

@tiny-book2 tiny-book2 changed the title allow to_ocaml to takes owned self allow to_ocaml to take owned self Oct 12, 2023
@tiny-book2
Copy link
Author

(I think this doesn't quite work, but thought I'd at least open the conversation)

@tiny-book2
Copy link
Author

Ok, with some extra changes, I think this largely works?? Was able to update polars-ocaml to use it in an experimental branch: mt-caret/polars-ocaml#87

@tizoc
Copy link
Owner

tizoc commented Oct 13, 2023

Hi. I have to take a deeper look when I have time, but at first sight I am not very convinced about the change.

i think this allows users to implement more efficient conversions in some cases --- for example, perhaps an owned String could be converted into a Bigarray without copying?

Totally agree that this is useful, but it is not what most Rust->OCaml conversions do right? such cases are rarer and are doing something more delicate by re-using in OCaml memory that lives outside of OCaml's heap, so I would rather have for such cases a different IntoOCaml trait with a well documented (with potential pitfalls) into_ocaml method.

@tiny-book2
Copy link
Author

Thanks for taking a look at my wacky idea! That sounds like a good idea; I'll make it when I get a free moment.

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.

2 participants