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 to/from casting for extern w/ no allocation #2208

Merged
merged 6 commits into from
Apr 6, 2021

Conversation

MarkMcCaskey
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey commented Mar 23, 2021

Resolves #2197

This should be good to go now.

The only question is how we want to deal with a breaking change this big. @Hywan and I previously discussed this, it seems other implementers of the wasm C API may have the same bug this is fixing

New Changelog rendered
Primary Changelog rendered

@MarkMcCaskey MarkMcCaskey requested a review from Hywan March 23, 2021 16:02
Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Thanks for tackling this :-).

@MarkMcCaskey MarkMcCaskey marked this pull request as ready for review March 31, 2021 16:45
@MarkMcCaskey MarkMcCaskey requested a review from jubianchi as a code owner March 31, 2021 16:45
Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well done!

I see you've updated the tests. Did you check the doctests too?

I'm approving the PR despites there is dbg! calls that need to be removed :-).

Comment on lines +19 to +27
/// All elements in this union must be `repr(C)` and have a
/// `CApiExternTag` as their first element.
#[allow(non_camel_case_types)]
pub(crate) union wasm_extern_inner {
function: mem::ManuallyDrop<wasm_func_t>,
memory: mem::ManuallyDrop<wasm_memory_t>,
global: mem::ManuallyDrop<wasm_global_t>,
table: mem::ManuallyDrop<wasm_table_t>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


#[allow(non_camel_case_types)]
#[derive(Clone)]
#[repr(transparent)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

lib/c-api/src/wasm_c_api/externals/mod.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/externals/mod.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/externals/mod.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/externals/mod.rs Outdated Show resolved Hide resolved
@MarkMcCaskey MarkMcCaskey changed the title Add wip solution to wasm_extern_as_ no allocation Implement to/from casting for extern w/ no allocation Apr 2, 2021
@MarkMcCaskey
Copy link
Contributor Author

bors r+

@MarkMcCaskey
Copy link
Contributor Author

bors r-

@bors
Copy link
Contributor

bors bot commented Apr 6, 2021

Canceled.

@MarkMcCaskey
Copy link
Contributor Author

bors r+

@bors bors bot merged commit 640d9bb into master Apr 6, 2021
@bors bors bot deleted the fix/c-api-allocation branch April 6, 2021 16:13
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.

Mismatch with Wasm C API on wasm_extern_as_* functions return value
2 participants