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.

Sign up for GitHub

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

Jump to bottom

feat(runtime-core) Implement TryFrom<native_type> for Value #1223

Merged
merged 2 commits into from
Feb 18, 2020
Merged

feat(runtime-core) Implement TryFrom<native_type> for Value #1223

merged 2 commits into from
Feb 18, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Feb 17, 2020

Extracted from #1018.

This PR implements TryFrom for Value. It is required to support dynamically-typed values for polymorphic host functions.

This PR also refactors the code by using a macro (value_conversions!) to implement From and TryFrom in one shot.

@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates labels Feb 17, 2020
MarkMcCaskey
MarkMcCaskey approved these changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks fine to me! I'm unsure what the best practices are for implementing TryFrom on types that can never fail 🤔 . I'd be fine shipping this but I have a preference whatever the wider Rust ecosystem does (which I don't currently know) to minimize the amount of surprises in our APIs.

edit: why, for example, would we not use the Infallible error type in these TryFroms?

@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2020

Precisely, it can fail. For instance, let v be a Value. You want a f32 but it's a I32. You will use try_into() and it might fail.

@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 18, 2020
@bors @Hywan
Merge #1223
3d4d6eb 
1223: feat(runtime-core) Implement `TryFrom<native_type>` for `Value` r=Hywan a=Hywan

Extracted from #1018.

This PR implements `TryFrom` for `Value`. It is required to support dynamically-typed values for polymorphic host functions.

This PR also refactors the code by using a macro (`value_conversions!`) to implement `From` and `TryFrom` in one shot.

Co-authored-by: Ivan Enderlin <[email protected]>
Co-authored-by: Ivan Enderlin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 18, 2020

Build succeeded

@bors bors bot merged commit e9333c5 into wasmerio:master Feb 18, 2020
@MarkMcCaskey
Copy link
Contributor

Precisely, it can fail. For instance, let v be a Value. You want a f32 but it's a I32. You will use try_into() and it might fail.

Oh didn't realize it was bidirectional conversion, I thought it was the same conversion implemented with both From and TryFrom!

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey approved these changes

@losfair losfair Awaiting requested review from losfair

Assignees
No one assigned
Labels
🎉 enhancement New feature! 📦 lib-deprecated About the deprecated crates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hywan @MarkMcCaskey