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

feat(types) Avoid allocating a Vec when calling FunctionType::new #2141

Merged

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Feb 25, 2021

Description

Sequel of #2036. We can go further by defining Params and Results of FunctionType::new as Into<Box<[Type]>> directly rather than Into<Vec<Type>>. It simplifies the code: params.into() rather than params.into().into_boxed_slice(). I assume it doesn't allocate an intermediate vector.

Since From<Box<[T]>> is implemented for Vec<T>, I reckon it's not a BC break.

Review

  • Add a short description of the the change to the CHANGELOG.md file not necessary

@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-types About wasmer-types labels Feb 25, 2021
@Hywan Hywan requested a review from MarkMcCaskey February 25, 2021 12:54
@Hywan Hywan self-assigned this Feb 25, 2021
@Hywan Hywan requested a review from syrusakbary as a code owner February 25, 2021 12:54
@Hywan
Copy link
Contributor Author

Hywan commented Feb 25, 2021

bors try

@Hywan
Copy link
Contributor Author

Hywan commented Feb 26, 2021

bors r+

bors bot added a commit that referenced this pull request Feb 26, 2021
2141: feat(types) Avoid allocating a `Vec` when calling `FunctionType::new` r=Hywan a=Hywan

# Description

Sequel of #2036. We can go further by defining `Params` and `Results` of `FunctionType::new` as `Into<Box<[Type]>>` directly rather than `Into<Vec<Type>>`. It simplifies the code: `params.into()` rather than `params.into().into_boxed_slice()`. I assume it doesn't allocate an intermediate vector.

Since `From<Box<[T]>>` is implemented for `Vec<T>`, I reckon it's not a BC break.

# Review

- [ ] ~Add a short description of the the change to the CHANGELOG.md file~ not necessary


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

bors bot commented Feb 26, 2021

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Feb 26, 2021

The CI failure has nothing related to this PR. Weird.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Since From<Box<[T]>> is implemented for Vec, I reckon it's not a BC break.

From<Box<[T]>> for Vec<T> means that we can go from Box<T> -> Vec<T>, did you mean Into<Box<[T]>> for Vec<T>? If so that's better but I think it's still a breaking change because for a given type GT, if GT implements Into<Vec<T>> but not Into<Box<[T]>> the caller code now has to do gt.into().into() (or because we call one of the intos they'd have to change from FunctionType::new(g, ...) to FunctionType::new(g.into(), ...)

I think this change is probably a good one but I do think it's probably a breaking change. We have lots of (generally very minor like this) breaking changes coming anyways though, we just need to decide when we want to ship a release to get them all in

Comment on lines -148 to -161
enum SecretOption {
None,
Some,
}

impl fmt::Debug for SecretOption {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Self::None => write!(f, "None"),
Self::Some => write!(f, "Some(...)"),
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems entirely unrelated. Not sure if you intended to commit it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's deadcode, raising a warning from rustc.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

approved barring breaking changes, as you mentioned we still need to figure out how to manage them

@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 2, 2021

@bors bors bot merged commit 77a35a3 into wasmerio:master Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-types About wasmer-types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants