-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add support for map type #12216
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
base: main
Are you sure you want to change the base?
Add support for map type #12216
Conversation
6b6f675 to
cd2282b
Compare
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api", "wasmtime:config", "wizer"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
|
On a skim this looks like it's all in the right direction, thanks! As as a heads up the wasm-tools deps will be updated in #12254 which'll avoid the need for git deps. I'll take a closer look once this is further along in CI passing tests |
db45a3a to
83dd7a7
Compare
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
83dd7a7 to
b9ca740
Compare
…model This commit introduces the Map and MapEntry classes, enabling the representation of map values in the component model. The Map class allows for the creation and iteration of key/value pairs, enhancing the functionality of the wasmtime component API. Additionally, the .gitignore file is updated to exclude build artifacts from the crates/c-api directory.
| vendor | ||
| examples/build | ||
| examples/.cache | ||
| crates/c-api/build |
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.
Is it ok to do this? I kept getting things to commit locally, so I added this
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.
Yeah this is fine to have here, no worries
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.
This is looking quite good to me, thanks for the thorough tests!
I haven't scrutinized the trampoline generation nor the lifting/lowering yet, but I can do that once the tests added here are passing (the #[ignore] ones at least).
If you can one thing I'd also recommend is modeling as many tests as possible as a *.wast test since that's generally the easiest to run and share (albeit difficult to write and debug)
|
|
||
| let mut config = Config::new(); | ||
| config.wasm_component_model(true); | ||
| config.wasm_features(wasmtime::WasmFeatures::CM_MAP, true); |
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.
Could this get scoped to just the tests in question using this feature?
| // Note: Map types (case 21) are disabled because HashMap doesn't implement | ||
| // ComponentType, Lift, or Lower yet. When those implementations are added, | ||
| // change this to 21. |
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.
Is this still applicable?
| (type (map u32 string)) | ||
| (type (map string u32)) | ||
| (type (map u32 u32)) | ||
| (type (map string (map u32 string))) |
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.
Mind splitting this out to a separate test file to avoid gating this preexsting test on the feature? It's not 100% relevant to us insofar as Wasmtime can put all the tests in one place, but in the interest of one day sharing tests with other runtimes it might be good to split things out by feature
| vendor | ||
| examples/build | ||
| examples/.cache | ||
| crates/c-api/build |
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.
Yeah this is fine to have here, no worries
Context
This is adding support for
mapbased on WebAssembly/component-model#554References