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
-->
fitzgen requested alexcrichton for a review on PR #8326.
fitzgen requested wasmtime-core-reviewers for a review on PR #8326.
fitzgen updated PR #8326.
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>
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.h
header feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafe
and 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.
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.h
header feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafe
and 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.
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.
fitzgen submitted PR review.
fitzgen created PR review comment:
Split out the renaming to https://github.com/bytecodealliance/wasmtime/pull/8344
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.h
header feels "too incompatible" with our implementation, to the extent that I'm pretty worried about the self-references andunsafe
and 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.
fitzgen closed without merge PR #8326.
Last updated: Dec 23 2024 at 12:05 UTC