rusterlium / erlang_nif-sys

Low level bindings to Erlang NIF API for Rust
Apache License 2.0
90 stars 19 forks source link

Support `enif_snprintf` and other variadic functions. #23

Closed jorendorff closed 7 years ago

jorendorff commented 7 years ago

Rust does support ... for extern "C" functions. I'll use this in Rustler to implement Debug for Erlang terms.

goertzenator commented 7 years ago

On the Windows side of things there is a Rust wrapper for each enif_xxx function and those can't be variadic. The wrappers dig a function pointer out of a big static struct to invoke the actual API function.

Thinking aloud... Could we keep enif_snprintf hidden and add a dumb wrapper that calls it? Underlying impl for Windows and Unix would be different. The wrapper could be fixed to print just a single term... that's all we need to get Debug and/or Display.

goertzenator commented 7 years ago

Let me know what you think about that if you would like me to take a swing at it.

jorendorff commented 7 years ago

I was going to hold off responding until I understood why things are that way on Windows.

jorendorff commented 7 years ago

But yes, a function that just sprints one term would be sufficient for my purpose.

goertzenator commented 7 years ago

Here's the Windows expansion in case you don't have a Windows Rust platform handy.

jorendorff commented 7 years ago

Thanks. But why is it like that? Is there something about how DLLs work on Windows that makes it impossible for a DLL to depend on symbols defined in the EXE that loads it? Or is it something peculiar to Erlang?

What if we expose these functions as macros? I bet that could be made to work on all platforms. On Windows, enif_sprintf!(ARGS) would expand to something like $crate::get_enif_sprintf_function_pointer()(ARGS). On other platforms, $crate::enif_sprintf(ARGS).

I don't have a great way to test this, though.

goertzenator commented 7 years ago

That's a Windows thing; you basically have to do back linkage by hand at runtime as is happening in our case.

Macros would indeed work. Namespacing for macros is a bit ugly right now, but apparently that will be fixed when macros 2.0 land (maybe next year).

Vararg functions are footguns; I still have a slight preference for hiding enif_snprintf() behind a limited rust function like:

fn print_term(&mut [u8], ERL_NIF_TERM) -> bool {}

... but I don't feel that strongly about it. I can patch up the windows side of the path you choose since I have that environment handy.

jorendorff commented 7 years ago

Well, I guess I think erlang_nif-sys should aim to be a minimal wrapper of the C API, even if it's a footgun. It's not a strong opinion on my side either; if I had read your last comment before writing the patch, I might have done it that way instead.

In Rustler, I don't plan on writing that particular safe function. Instead, I'm going to be pointing snprintf at an uninitialized buffer created using Vec::with_capacity(), like in the test I wrote. So for me it's better to have the footgun.

jorendorff commented 7 years ago

Oh, it looks like my test assumes 2.11, and the build server doesn't have it. I can write a test for enif_make_list instead. But what can I do about this test? Just delete it?

goertzenator commented 7 years ago

Sorry for the delay on this.

Strange. Appveyor is using OTP-19.2 which should be 2.11. I'll dig in on the windows side.

goertzenator commented 7 years ago

It tests fine on my Windows machine. Purely an appveyor config issue.