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 try_from instead of the cast crate. #172

Merged
merged 3 commits into from
Jun 9, 2019
Merged

Use try_from instead of the cast crate. #172

merged 3 commits into from
Jun 9, 2019

Conversation

arilotter
Copy link
Contributor

try_from is stable now, so cast is unnecessary.
This closes #169

Copy link
Member

@kubkon kubkon 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! It looks like there are 2 more crates which could use converting from cast to try_from: wasmtime-wasi-c and wasmtime-runtime. Do you think you'd be able to make those changes in this PR? :-)

@arilotter
Copy link
Contributor Author

@kubkon I've converted those two crates too!
I notice there were places in the code that had the pattern

#[cfg(target_pointer_width = "32")]
let sig_id = VMSharedSignatureIndex::new(cast::u32(len));
#[cfg(target_pointer_width = "64")]
let sig_id = VMSharedSignatureIndex::new(cast::u32(len).unwrap());

I assume that this is written as such because the cast crate does conditional compilation too, and provides a different signature based on the target pointer size - Is there any problem with doing an unconditional unwrap, since the try_from trait always provides a Result?

@kubkon
Copy link
Member

kubkon commented Jun 9, 2019

@kubkon I've converted those two crates too!
I notice there were places in the code that had the pattern

#[cfg(target_pointer_width = "32")]
let sig_id = VMSharedSignatureIndex::new(cast::u32(len));
#[cfg(target_pointer_width = "64")]
let sig_id = VMSharedSignatureIndex::new(cast::u32(len).unwrap());

I assume that this is written as such because the cast crate does conditional compilation too, and provides a different signature based on the target pointer size

Correct!

Is there any problem with doing an unconditional unwrap, since the try_from trait always provides a Result?

None at all. To the best of my knowledge, we can safely chuck away the cfgs here and just have a single version using try_from.

@arilotter
Copy link
Contributor Author

Alright, then this should be ready to merge 😁 I made sure to cargo fmt too

@kubkon
Copy link
Member

kubkon commented Jun 9, 2019

Looks great, thanks! For the future, you can run format-all.sh to do the formatting for you, and test-all.sh to check the formatting, builds, and run some basic spec tests ;-)

@kubkon kubkon merged commit 8dc1d90 into bytecodealliance:master Jun 9, 2019
pchickey added a commit to pchickey/wasmtime that referenced this pull request May 12, 2023
* delete adapter src/main.o: this was accidentally left out of bytecodealliance#165

* move adapter, byte-array, and verify to a new workspace

* rename byte-array crate to a name available on crates.io

* add a readme for verify, also give it a slightly better name

* CI: wit dep check in its own step, verify before publish, trim down publication

* reactor-tests: delete deps symlinks

* reactor-tests: manage wit with wit-deps

* test: dont set default toolchain to nightly

* wit-deps lock adapter

* wit-deps lock reactor-tests

wit-deps doesnt manage these for some reason
pchickey added a commit to pchickey/wasmtime that referenced this pull request May 16, 2023
* delete adapter src/main.o: this was accidentally left out of bytecodealliance#165

* move adapter, byte-array, and verify to a new workspace

* rename byte-array crate to a name available on crates.io

* add a readme for verify, also give it a slightly better name

* CI: wit dep check in its own step, verify before publish, trim down publication

* reactor-tests: delete deps symlinks

* reactor-tests: manage wit with wit-deps

* test: dont set default toolchain to nightly

* wit-deps lock adapter

* wit-deps lock reactor-tests

wit-deps doesnt manage these for some reason
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.

Use try_from
2 participants