-
Notifications
You must be signed in to change notification settings - Fork 257
Initial fixed-length list implementation #1277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I have a newer version at https://github.com/cpetig/wit-bindgen/tree/work-in-progress which tests more in depth and also supports C++, I need to separate and clean up that one and then push it here for review. |
|
Also this requires bytecodealliance/wasmtime#10619 , and a very recent version of wasm-component-ld for the tests to pass. |
7f2f236 to
ed27ed6
Compare
ed27ed6 to
e7ce6f0
Compare
|
This is ready, but depends on bytecodealliance/wasmtime#10619 for successful tests. |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
This still has a limitation: The generated Rust and C++ code can't handle non-Copy/POD types. For Functional Safety this isn't an issue as you need to avoid putting string or list into a fixed size array anyway, resources might still make some sense. I created #1501 to track this limitation. |
|
TODO: Check for correct usage of "fixed-length" over "fixed-size" |
e7ce6f0 to
4f86bd4
Compare
|
Second thought: The impact of the wasm-tools change will be identical (all code generators in wit-bindgen have to do a search and replace independently of whether this patch is merged or not). But it will create a conflict with this patch, so I guess it can be merged and then the other change can be discussed separately. |
|
The (discussion of the) rename in wasm-tools happens in bytecodealliance/wasm-tools#2421 |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and get this merged and renames/refactors can land in parallel
See WebAssembly/component-model#384 for the commit which added fixed size lists to the component model standard documents.