Skip to content

Conversation

@MangoPeachGrape
Copy link
Contributor

Only get_func() as of now, should #9812 (comment) come in a later PR?

One thing of note, if name isn't UTF-8, it returns false instead of returning an error, is that fine?

@MangoPeachGrape MangoPeachGrape requested a review from a team as a code owner April 25, 2025 20:00
@MangoPeachGrape MangoPeachGrape requested review from dicej and removed request for a team April 25, 2025 20:00
@alexcrichton alexcrichton requested review from alexcrichton and removed request for dicej April 25, 2025 20:44
@alexcrichton
Copy link
Member

Thanks! This has triggered some further thoughts which are both preexisting and related to this PR as well:

  • For names I realize now we shouldn't use const char*, but instead we should be using wasm_name_t*. Unicode can't be represented as const char* due to the lack of ability to support interior nul characters, so we'll want to change this and other APIs to use wasm_name_t instead.
  • For this I think we'll want to base lookup of an item on an ComponentExportIndex instead of a string, although a string can be used to acquire an ComponentExportIndex. Effectively I think we should bind {Instance,Component}::get_export_index and then an export index is used to lookup a function. That'll handle nested instances as well as top-level exports in the same manner.

@alexcrichton
Copy link
Member

Er, sent too soon:

One thing of note, if name isn't UTF-8, it returns false instead of returning an error, is that fine?

Yeah sounds good!


One other slightly orthogonal thing, I think it'd be nice to help clean up the C tests a bit. If you'd like I think it'd be reasonable to export the C API implementation pointer from C++ structures, that way you could sort of intermix C++ and C where the C++ APIs could be used to handle auto-destruction and such. That might also make it easier to bind optional errors and assert that errors don't happen. Otherwise could the CHECK macro be deduplicated in a header instead of duplicated between files?

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 25, 2025
@MangoPeachGrape MangoPeachGrape force-pushed the c-api/component-model/get-func branch from f4a7766 to 1814837 Compare April 28, 2025 17:25
@alexcrichton alexcrichton added this pull request to the merge queue Apr 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 28, 2025
@MangoPeachGrape
Copy link
Contributor Author

Next up values, which are required for function invocation?

@alexcrichton alexcrichton enabled auto-merge April 28, 2025 19:20
@alexcrichton
Copy link
Member

Yeah that seems reasonable. I'll be honest though in that I don't really know how to do values. Everything I've thought of historically is either extremely chatty over the C API boundary or extremely allocation-heavy, neither of which I feel is great... If you've got ideas though please dive in!

@alexcrichton alexcrichton added this pull request to the merge queue Apr 28, 2025
Merged via the queue into bytecodealliance:main with commit ce3c1a7 Apr 28, 2025
43 checks passed
@MangoPeachGrape MangoPeachGrape deleted the c-api/component-model/get-func branch April 28, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:c-api Issues pertaining to the C API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants