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

Implement NativeWasmTypeInto for u32 and u64 with the js feature. #3762

Closed
kajacx opened this issue Apr 11, 2023 · 2 comments
Closed

Implement NativeWasmTypeInto for u32 and u64 with the js feature. #3762

kajacx opened this issue Apr 11, 2023 · 2 comments
Assignees
Labels
🎉 enhancement New feature! priority-medium Medium priority issue
Milestone

Comments

@kajacx
Copy link
Contributor

kajacx commented Apr 11, 2023

Motivation

When using wasmer on the web (wasmer itself is compiled to WASM and then loads another WASM module) with the js feature, you cannot pass u32 and u64 as arguments from / to functions, because those types do not implement NativeWasmTypeInto.

Proposed solution

Implement NativeWasmTypeInto for u32 and u64.

Alternatives

You can, of course, cast your unsigned integers to signed integers, but it is kind of annoying. Example:

// Plugin:
#[no_mangle]
pub fn add_ten(value: u32) -> u32 {
    value + 10
}

// Host, current workaround:
let mut value: u32 = 50;
let add_ten = instance
    .exports
    .get_typed_function::<i32, i32>(&store, "add_ten")
    .unwrap();
value = take_u32.call(&mut store, value as i32).unwrap() as u32;

// Host, with proposed change:
let mut value: u32 = 50;
let add_ten = instance
    .exports
    .get_typed_function::<u32, u32>(&store, "add_ten")
    .unwrap();
value = take_u32.call(&mut store, value).unwrap();

Additional context

Not sure if implementing NativeWasmTypeInto for u8 / u16 / i8 / i16 makes sense in the same way, but that can be another feature request.

@kajacx kajacx added the 🎉 enhancement New feature! label Apr 11, 2023
@syrusakbary
Copy link
Member

That sounds good to me. Do you want to work on a PR @kajacx ?

kajacx added a commit to kajacx/wasmer that referenced this issue Apr 12, 2023
kajacx added a commit to kajacx/wasmer that referenced this issue Apr 12, 2023
kajacx added a commit to kajacx/wasmer that referenced this issue Apr 20, 2023
@theduke theduke added this to the v4.0 milestone May 9, 2023
@theduke theduke added the priority-medium Medium priority issue label May 9, 2023
kajacx added a commit to kajacx/wasmer that referenced this issue May 10, 2023
@ptitSeb ptitSeb self-assigned this May 17, 2023
@ptitSeb
Copy link
Contributor

ptitSeb commented May 17, 2023

PR merged, closing the ticket.

@ptitSeb ptitSeb closed this as completed May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! priority-medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

4 participants