Skip to content
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

Use typed indices via a new wasm-types crate #2049

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samestep
Copy link
Contributor

Following up on some conversation in #1985, this draft PR is a start on what it might look like to use typed indices throughout the whole codebase. I was originally planning to finish this before coming back to #1985, but it eventually became clear to me that this is considerably bigger than I expected.

In any case, I'm opening this PR to gauge whether there's any interest in going further down this path, or if I should just call this an interesting experiment and abandon it. There were some unintuitive cases, so maybe there is some potential value in these from a codebase documentation/consistency perspective? For example:

wasmparser::CanonicalFunction::ThreadSpawn { func_ty_index } => {
let func_ty = reencoder.type_index(func_ty_index);
section.thread_spawn(func_ty);
}

This code made me mark the func_ty_index field of the CanonicalFunction::ThreadSpawn variant as TypeIdx instead of ComponentTypeIdx, unlike all the fields in all the Resource variants. Is this correct? I don't know much about the component model, so maybe threads just happen to use core type indices while other similar-looking things use component type indices. Maybe it's clear from the spec (?), but it's not clear from the docstring:

/// A function which spawns a new thread by invoking the shared function.
ThreadSpawn {
/// The index of the function to spawn.
func_ty_index: u32,
},

Another random question I had while working on this: why do both wasm_encoder::NameSection::tag and wasm_encoder::NameSection::tags exist? Don't they do the same thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant