tizoc / ocaml-interop

OCaml<->Rust FFI with an emphasis on safety.
MIT License
135 stars 22 forks source link

Support functions with >= 6 arguments #49

Closed mt-caret closed 1 year ago

mt-caret commented 1 year ago

AFAICT currently ocaml_export doesn't really know about restrictions around the number of arguments. It would be pretty nice if ocaml_export notices that and generates the bytecode version of the function as well.

I'm happy to take a crack at this and create a PR if you think this is a reasonable thing to add.

tizoc commented 1 year ago

@mt-caret hello! Yes, you are correct, the current macro doesn't take into account bytecode functions (but you should be able to do the same thing that is done in C).

But it is indeed reasonable to add support for this, I am just not sure about what it should look like (somehow, the bytecode name for the function needs to be provided in the macro, so that the wrapper function can be generated too). Did you have anything in particular in mind?

mt-caret commented 1 year ago

I'm actually kind of a Rust noob so please correct me if I'm proposing something that's impossible, but I was thinking of just tacking on _bytecode to the name and exporting that as a quick-n-dirty thing we can do.

tizoc commented 1 year ago

but I was thinking of just tacking on _bytecode to the name and exporting that as a quick-n-dirty thing we can do.

The symbol for the bytecode version name needs to be provided explicitly because with these macros new symbols cannot be introduced. Other than that it shouldn't be complicated. Let me think about it.

mt-caret commented 1 year ago

I see, that makes sense. I was trying to manually work around this when I realized that ocaml-interop doesn't work as-is with bytecode compilation anyway due to https://github.com/tizoc/ocaml-interop/issues/34 (I think?), so I guess even if we had something like this it won't be as useful as I originally thought.

tizoc commented 1 year ago

@mt-caret it can be worked around by enabling the no-caml-startup feature in the crate, or setting the OCAML_INTEROP_NO_CAML_STARTUP environment variable.

See https://github.com/tizoc/ocaml-interop/pull/37/commits/aebbdc5be302b894ecb77faf1172f22fd034a5cf and the related discussions:

It is all kinda dirty and not ideal, my hope is to be able to improve all this as part of the upgrade for supporting OCaml 5 (which requires improvements to how the runtime handle is implemented and also domain locks).

tizoc commented 1 year ago

@mt-caret I am still unsure about the syntax, but see #50

mt-caret commented 1 year ago

Woah, this looks awesome! Thanks so much for the quick fix :)

tizoc commented 1 year ago

Just merged #50, closing this

mt-caret commented 1 year ago

Awesome, tyvm!

tizoc commented 1 year ago

@mt-caret uff, the moment I published the new version I noticed I am generating the wrong code, I will fix it and publish it as v0.9.1

tizoc commented 1 year ago

@mt-caret ok, just published v0.9.1 with the fixed expansion.