Wasmtime GitHub notifications bot (Mar 26 2020 at 22:43): Wasmtime GitHub notifications bot (Mar 26 2020 at 22:43):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
unsafeeverywhereThese 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_newchanged 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.
Wasmtime GitHub notifications bot (Mar 26 2020 at 22:43):alexcrichton requested peterhuene for a review on PR #1415.
Wasmtime GitHub notifications bot (Mar 26 2020 at 22:43):alexcrichton requested peterhuene and yurydelendik for a review on PR #1415.
Wasmtime GitHub notifications bot (Mar 26 2020 at 23:05):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
unsafeeverywhereThese 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_newchanged 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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):peterhuene submitted PR Review.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):peterhuene submitted PR Review.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):peterhuene created PR Review Comment:
Same comment as above regarding returning
boolfor this function.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):peterhuene created PR Review Comment:
I wonder if this API should return
boolto indicate it was successful rather than dropping the result?
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):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?
Wasmtime GitHub notifications bot (Mar 27 2020 at 00:13):peterhuene edited PR Review Comment.
Wasmtime GitHub notifications bot (Mar 27 2020 at 14:08):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
unsafeeverywhereThese 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_newchanged 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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 14:50):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
unsafeeverywhereThese 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_newchanged 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.
Wasmtime GitHub notifications bot (Mar 27 2020 at 14:50):alexcrichton merged PR #1415.
Last updated: Nov 22 2024 at 16:03 UTC