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 OptionIntoWasmAbi for `Option<&Closure>. #2191

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teymour-aldridge
Copy link
Contributor

I think this is the correct way to do it. I'm not sure about that though (my knowledge about WebAssembly is very limited).
This should resolve #2173.

@teymour-aldridge teymour-aldridge changed the title Implement IntoWasmAbi for `Option<&Closure>. Implement OptionIntoWasmAbi for `Option<&Closure>. Jun 6, 2020
@alexcrichton
Copy link
Contributor

Thanks! Can you be sure to add some tests for this as well?

@teymour-aldridge
Copy link
Contributor Author

teymour-aldridge commented Jun 8, 2020

Sure! Could you suggest what I should test for?

@alexcrichton
Copy link
Contributor

In general I'd just exercise this feature. Make sure Some and None get passed, the basic functionality still works, everything compiles, etc.

@teymour-aldridge
Copy link
Contributor Author

Cool! (thanks)

@ctaggart
Copy link
Contributor

@teymour-aldridge thanks for starting this! Any luck adding tests?

@teymour-aldridge
Copy link
Contributor Author

Hi, I've been a bit busy recently so haven't had time to work on this much. If you'd like to add some tests, please feel free.

@ctaggart
Copy link
Contributor

I added a test to tests/wasm/closures.rs, but I'm not sure how to run the tests. When I run cargo test --target wasm32-unknown-unknown, I have 141 tests pass, but it does not include the closure tests.

@ctaggart
Copy link
Contributor

I see. I was missing --features nightly.

@ctaggart
Copy link
Contributor

I added a basic test that you can see in #2211. Unfortunately, this does not fix it yet and I am not sure how to.

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.

implement OptionIntoWasmAbi for Option<&Closure>
3 participants