fitzgen opened 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.
fitzgen requested alexcrichton for a review on PR #1996.
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.
bjorn3 submitted PR Review.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Could you add a fixme to use the
write
method ofMaybeUninit
once it is stabilized?
bjorn3 created PR Review Comment:
Same here
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.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
For this I believe the C API avoids returning structs-by-value where possible, so could the
wasm_val_t
be an out-pointer?
alexcrichton created PR Review Comment:
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 forMaybeUninit::write
? That should help reduce the creep ofunsafe
into some of these functions.
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)
alexcrichton created PR Review Comment:
Technically I think this should be
extern fn(*mut c_void)
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
alexcrichton created PR Review Comment:
Could this delegate to
new_with_finalizer
below?
alexcrichton created PR Review Comment:
It seems like there's two levels of
Option
in this representation, both here and in handling ofOption<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?
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
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.
alexcrichton submitted PR Review.
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
fitzgen submitted PR Review.
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
externref
s, 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.
alexcrichton submitted PR Review.
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!
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.
fitzgen requested alexcrichton for a review on PR #1996.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
I think this syntax isn't implemented for MSVC? (according to CI)
alexcrichton created PR Review Comment:
Perhaps change this to
*mut
?
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.
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.
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.
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.
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.
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.
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.
fitzgen merged PR #1996.
Last updated: Dec 23 2024 at 13:07 UTC