Wasmtime GitHub notifications bot (Apr 10 2024 at 22:01): Wasmtime GitHub notifications bot (Apr 10 2024 at 22:01):fitzgen opened PR #8326 from fitzgen:gc-refs-in-c-api-again to bytecodealliance:main:
Does not add support for the
wasmtime_*functions yet.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Apr 10 2024 at 22:01):fitzgen requested alexcrichton for a review on PR #8326.
Wasmtime GitHub notifications bot (Apr 10 2024 at 22:01):fitzgen requested wasmtime-core-reviewers for a review on PR #8326.
Wasmtime GitHub notifications bot (Apr 10 2024 at 22:36):fitzgen updated PR #8326.
Wasmtime GitHub notifications bot (Apr 10 2024 at 22:44):github-actions[bot] commented on PR #8326:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:33):alexcrichton submitted PR review:
Personally I'm feeling pretty uneasy about this having a self-referencing store. This is exclusively for the
wasm_*API which none of ourwasmtime-*bindings are using as well. Given that my gut would actually be the opposite of what you're thinking which is to basically stop supporting reference types in thewasm_*APIs and only support them inwasmtime_*APIs. Thewasm.hheader feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafeand bugs here. Given the fact that we'll have a full implementation available withwasmtime_*I'd be tempted to tailor those to what we need and only have that for now.
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:33):alexcrichton submitted PR review:
Personally I'm feeling pretty uneasy about this having a self-referencing store. This is exclusively for the
wasm_*API which none of ourwasmtime-*bindings are using as well. Given that my gut would actually be the opposite of what you're thinking which is to basically stop supporting reference types in thewasm_*APIs and only support them inwasmtime_*APIs. Thewasm.hheader feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafeand bugs here. Given the fact that we'll have a full implementation available withwasmtime_*I'd be tempted to tailor those to what we need and only have that for now.
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:33):alexcrichton created PR review comment:
Could these renamings perhaps be split out either commit-wise or PR-wise? It's a bit difficult to disentangle what's going on here with simultaneous renamings.
Wasmtime GitHub notifications bot (Apr 11 2024 at 23:28):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 11 2024 at 23:28):fitzgen created PR review comment:
Split out the renaming to https://github.com/bytecodealliance/wasmtime/pull/8344
Wasmtime GitHub notifications bot (Apr 11 2024 at 23:30):fitzgen commented on PR #8326:
Personally I'm feeling pretty uneasy about this having a self-referencing store. This is exclusively for the
wasm_*API which none of ourwasmtime-*bindings are using as well. Given that my gut would actually be the opposite of what you're thinking which is to basically stop supporting reference types in thewasm_*APIs and only support them inwasmtime_*APIs. Thewasm.hheader feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafeand bugs here. Given the fact that we'll have a full implementation available withwasmtime_*I'd be tempted to tailor those to what we need and only have that for now.Yeah, that's fair. I have a branch with
wasmtime_*working, and it is indeed a lot cleaner thanwasm_*so I'll close this PR in favor of https://github.com/bytecodealliance/wasmtime/pull/8344 and then open another new PR on top of that one with thewasmtime_*APIs.
Wasmtime GitHub notifications bot (Apr 11 2024 at 23:30):fitzgen closed without merge PR #8326.
Last updated: Nov 22 2024 at 16:03 UTC