alexcrichton opened PR #1415 from refactor-c-api
to master
:
This is a pretty huge PR which refactors and rewrites a large part of the C API's internals. The goals from this PR are:
- Fill out more C API surface area
- Organize the C API Rust source code to be easier to read/write
- Improve safety of the C API to avoid resorting to using
unsafe
everywhereThese are each tackled in a variety of the commits below, and I'm willing to go into anything in more detail of course!
Very few functional changes are intended as part of this PR. The only one I know of is that
wasm_functype_new
changed semantics where it takes ownership of the param/result vector memory and the types that the vectors point to. Previously it would only free the vector memory and it would not free the types that the vector memory points to.Reviewing this as a whole is unfortunately going to be relatively difficult. We don't have a huge amount of tests for the C API but I'm hoping that we can use what we have to get some coverage at least. In addition to the .NET extension in-tree using the C API I'm working on a python extension rewrite out of tree, and I've gotten all those current tests passing with this which gives me some degree of confidence at least. In theory though all the C API functions are pretty bite-sized and should be easy to skim over, but I may have to also mostly be trusted that I didn't change too too much while I was moving things around here.
alexcrichton requested peterhuene for a review on PR #1415.
alexcrichton requested peterhuene and yurydelendik for a review on PR #1415.
alexcrichton updated PR #1415 from refactor-c-api
to master
:
This is a pretty huge PR which refactors and rewrites a large part of the C API's internals. The goals from this PR are:
- Fill out more C API surface area
- Organize the C API Rust source code to be easier to read/write
- Improve safety of the C API to avoid resorting to using
unsafe
everywhereThese are each tackled in a variety of the commits below, and I'm willing to go into anything in more detail of course!
Very few functional changes are intended as part of this PR. The only one I know of is that
wasm_functype_new
changed semantics where it takes ownership of the param/result vector memory and the types that the vectors point to. Previously it would only free the vector memory and it would not free the types that the vector memory points to.Reviewing this as a whole is unfortunately going to be relatively difficult. We don't have a huge amount of tests for the C API but I'm hoping that we can use what we have to get some coverage at least. In addition to the .NET extension in-tree using the C API I'm working on a python extension rewrite out of tree, and I've gotten all those current tests passing with this which gives me some degree of confidence at least. In theory though all the C API functions are pretty bite-sized and should be easy to skim over, but I may have to also mostly be trusted that I didn't change too too much while I was moving things around here.
peterhuene submitted PR Review.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
Same comment as above regarding returning
bool
for this function.
peterhuene created PR Review Comment:
I wonder if this API should return
bool
to indicate it was successful rather than dropping the result?
peterhuene created PR Review Comment:
I'm off the opinion that we should probably just close WebAssembly/wasm-c-api#126 and remove this FIXME.
peterhuene created PR Review Comment:
I know the C API is thoroughly undocumented regarding return values, but it does seem to lean towards returning null in situations like this, rather than panic.
peterhuene created PR Review Comment:
Should we file a bug on the C API repo for this? Seems like if there's a type mismatch or if the global isn't mutable that there should be a way of communicating the problem to the caller.
peterhuene created PR Review Comment:
I doubt there would ever be more than 0 and 1 as values, but do we want symbolic representation of the C enum rather than the hardcoded values here and in
wasm_globaltype_mutability
?
peterhuene edited PR Review Comment.
alexcrichton updated PR #1415 from refactor-c-api
to master
:
This is a pretty huge PR which refactors and rewrites a large part of the C API's internals. The goals from this PR are:
- Fill out more C API surface area
- Organize the C API Rust source code to be easier to read/write
- Improve safety of the C API to avoid resorting to using
unsafe
everywhereThese are each tackled in a variety of the commits below, and I'm willing to go into anything in more detail of course!
Very few functional changes are intended as part of this PR. The only one I know of is that
wasm_functype_new
changed semantics where it takes ownership of the param/result vector memory and the types that the vectors point to. Previously it would only free the vector memory and it would not free the types that the vector memory points to.Reviewing this as a whole is unfortunately going to be relatively difficult. We don't have a huge amount of tests for the C API but I'm hoping that we can use what we have to get some coverage at least. In addition to the .NET extension in-tree using the C API I'm working on a python extension rewrite out of tree, and I've gotten all those current tests passing with this which gives me some degree of confidence at least. In theory though all the C API functions are pretty bite-sized and should be easy to skim over, but I may have to also mostly be trusted that I didn't change too too much while I was moving things around here.
alexcrichton updated PR #1415 from refactor-c-api
to master
:
This is a pretty huge PR which refactors and rewrites a large part of the C API's internals. The goals from this PR are:
- Fill out more C API surface area
- Organize the C API Rust source code to be easier to read/write
- Improve safety of the C API to avoid resorting to using
unsafe
everywhereThese are each tackled in a variety of the commits below, and I'm willing to go into anything in more detail of course!
Very few functional changes are intended as part of this PR. The only one I know of is that
wasm_functype_new
changed semantics where it takes ownership of the param/result vector memory and the types that the vectors point to. Previously it would only free the vector memory and it would not free the types that the vector memory points to.Reviewing this as a whole is unfortunately going to be relatively difficult. We don't have a huge amount of tests for the C API but I'm hoping that we can use what we have to get some coverage at least. In addition to the .NET extension in-tree using the C API I'm working on a python extension rewrite out of tree, and I've gotten all those current tests passing with this which gives me some degree of confidence at least. In theory though all the C API functions are pretty bite-sized and should be easy to skim over, but I may have to also mostly be trusted that I didn't change too too much while I was moving things around here.
alexcrichton merged PR #1415.
Last updated: Dec 23 2024 at 13:07 UTC