wasmtime / PR #1996 Support reference types in the C API · git-wasmtime

I think that writing to &mut MaybeUninit<T> is actually a safe operation, so could the unsafety be encapsulated in a helper method for now while we wait for MaybeUninit::write? That should help reduce the creep of unsafe into some of these functions.

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

FWIW the other APIs with finalizers don't assign a type name to the finalizer, they'd just do void(*finalizer)(void*) inline (although naturally this is much more readable)

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

Technically I think this should be extern fn(*mut c_void)

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

This I think could be &mut *mut c_void to improve safety a bit (and document at the type level that it cant be null

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

Could this delegate to new_with_finalizer below?

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

It seems like there's two levels of Option in this representation, both here and in handling of Option<Box<wasm_ref_t>>. Is there a way that we could perhaps unify that so there's only one level at which null is introduced?

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

Is this method still used in the API somewhere? This seems like it's probably unsafe to do since it runs the dtor on the previous contents and I don't think that there's anywhere else we should do that in the API

Wasmtime GitHub notifications bot (Jul 09 2020 at 14:16):

alexcrichton created PR Review Comment:

I think this is a bit surprising to do here because this means that you can get back pointers to externref values that you didn't create. I would expect null to be returned here, although I can't imagine this'll be too common of a case to hit since they're no way in the API to create an externref other than the functions above.

Wasmtime GitHub notifications bot (Jul 09 2020 at 20:57):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 09 2020 at 20:57):

alexcrichton created PR Review Comment:

FWIW I think it might be best from an API perspective to not take ownership here, since it's not really needed anyway

Wasmtime GitHub notifications bot (Jul 10 2020 at 18:38):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 18:38):

fitzgen created PR Review Comment:

I was also split on what the best thing to do here was.

I was imagining that someone might have both Rust code and C code that try to communicate via externrefs, and have some out of band protocol for communicating the types of values they are sending each other.

I can remove it, and return null in this case, if you think that is the better course of action.

Wasmtime GitHub notifications bot (Jul 10 2020 at 18:53):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 18:53):

alexcrichton created PR Review Comment:

I suppose if there's multiple actors in play you can't really assume anything about the externref pointers you get back anyway. Given two C embeddings there's no guarantee their pointers align, so seems fine to just return what we can here for now. Always possible to tweak later!

Wasmtime GitHub notifications bot (Jul 10 2020 at 19:05):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 10 2020 at 19:06):

fitzgen requested alexcrichton for a review on PR #1996.

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:06):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:06):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:06):

alexcrichton created PR Review Comment:

I think this syntax isn't implemented for MSVC? (according to CI)

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:06):

alexcrichton created PR Review Comment:

Perhaps change this to *mut?

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:07):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:09):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 10 2020 at 20:55):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 10 2020 at 21:12):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 13 2020 at 15:58):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 13 2020 at 16:14):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 13 2020 at 16:35):

fitzgen updated PR #1996 from ref-types-in-c-api to main:

See each commit message for details. The first commit in particular has a bit of rippling out due to wasm_val_t not being POD anymore.

Wasmtime GitHub notifications bot (Jul 13 2020 at 17:58):

fitzgen merged PR #1996.

Last updated: Nov 22 2024 at 17:03 UTC