search

rusterlium / erlang_nif-sys

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

ErlNifEnv mutability changes #25

Closed goertzenator closed 7 years ago

goertzenator commented 7 years ago

This case proposes two changes around ErlNifEnv:

  1. Remove ErlNifEnv’s Sync trait. ErlNifEnv is an opaque type and it’s initial naive implementation happens to be Sync. The NIF documentation indicates it is not thread safe, therefore it should not have Sync. Send is okay though.

  2. This one is more radical: Change usages of *mut ErlNifEnv to *const ErlNifEnv in all API functions. Both Ruster and Rustler use const references in their env types because it better represents the allowed usages of ErlNifEnv. Changing the mutability of the underlying pointer in erlang_nif-sys would give other users a better cue of how to use that type, and allow less transmuting/wrapping between erlang_nif-sys and Ruster/Rustler. I should add that ErlNifEnv is “exteriorly immutable”; there are no operations on it that visibly modify it. I should also point out that removal of the Sync trait tightens any looseness const references may have had in the past.

A quick look and Rustler shows that a tweak to the NIF_ENV is all that is needed to adapt to this change. @hansihe and @jorendorff , please advise if you object to this change or if you think I've misunderstood something. Thanks.

jorendorff commented 7 years ago

I agree with removing Sync from ErlNifEnv.

In general, I don't think mut and const on Rust pointers as carrying any of the meaning of Rust mut and non-mut references. In terms of what Rust itself allows you to do with pointers, they are more like C++ non-const and const pointers, so I think it's better for the signatures in erlang_nif-sys to reflect what the C header says.

goertzenator commented 7 years ago

Sync removed from ErlNifEnv in ddb7a66d8b8a5c011becb0d2012bc5885023e8e4