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) Remove dependency to cranelift-entity #2195

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Mar 18, 2021

Description

Don't be afraid, everything is cool.

cranelift-entity is nice, but we are limited by it. We want to go
further for some features. Those features are very likely to be
refused by Cranelift because it's out of their scope.

This patch basically copies the code from cranelift-entity inside
wasmer-types. The code has been modified to fit in our own
no_std” strategy. The code will be modified deeper in upcoming
patches and most of the entities are likely to radically change. The
goal of this patch is strictly to not break our code while porting the
new code.

Review

  • Add a short description of the the change to the CHANGELOG.md file
  • Do we need to update the ATTRIBUTIONS.md file?

Hywan added 3 commits March 18, 2021 14:05
Don't be afraid, everything is cool.

`cranelift-entity` is nice, but we are limited by it. We want to go
further for some features. Those features are very likely to be
refused by Cranelift because it's out of their scope.

This patch basically copies the code from `cranelift-entity` inside
`wasmer-types`. The code has been modified to fit in our own
“`no_std`” strategy. The code will be modified deeper in upcoming
patches and most of the entities are likely to radically change. The
goal of this patch is strictly to not break our code while porting the
new code.
@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-types About wasmer-types labels Mar 18, 2021
@Hywan Hywan self-assigned this Mar 18, 2021
@Hywan
Copy link
Contributor Author

Hywan commented Mar 18, 2021

bors try

bors bot added a commit that referenced this pull request Mar 18, 2021
@bors
Copy link
Contributor

bors bot commented Mar 18, 2021

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Mar 18, 2021

bors try

bors bot added a commit that referenced this pull request Mar 18, 2021
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.

Looks good to me.

👍 on the attributions. I'm in favor of being explicit about where the code came from and what the license is

lib/wasmer-types/src/entity/mod.rs Show resolved Hide resolved
lib/wasmer-types/src/entity/packed_option.rs Outdated Show resolved Hide resolved
lib/wasmer-types/src/entity/packed_option.rs Show resolved Hide resolved
}

/// Get the element at `k` if it exists.
#[inline(always)]
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit odd to see these on SecondaryMap and not on PrimaryMap but 🤷 . I find it easy to believe that these should always be inlined and that Rust probably also agrees and doesn't need to be told.

Though I think the #[inline] attributes do matter for cross-crate inlining without LTO? With LTO they don't matter, but without LTO they do. At least that's what I remember, not super confident in that memory though.

Anyways, no action needed here. Just pointing it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That's part of the rewrite I want to do later. Let's keep the same code for the moment, and let's optimize later as planned :-). Thanks for pointing that out!

@bors
Copy link
Contributor

bors bot commented Mar 18, 2021

try

Build failed:

@syrusakbary
Copy link
Member

syrusakbary commented Mar 18, 2021

Note: on the attributions we should follow our current structure. Which is:

  • No need to change ATTRIBUTIONS.md since the following is still true: wasmtime: for their API and internal documentation, as well as some internal implementations
  • We need to update wasmer-types/README.md adding an Acknowledgments section with the cranelift-entity reference.
### Acknowledgments

This project borrowed some of the code for the entity structure from [cranelift-entity](https://crates.io/crates/cranelift-entity).
We decided to move it here to help on serialization/deserialization and also to ease the integration with other tools like loupe.

Please check [Wasmer ATTRIBUTIONS](https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md) to further see licenses and other attributions of the project. 
  • We need the following content to the headers in the new added files:
// This file contains code from external sources.
// Attributions: https://github.com/wasmerio/wasmer/blob/master/ATTRIBUTIONS.md

@Hywan
Copy link
Contributor Author

Hywan commented Mar 18, 2021

bors r+

@bors bors bot merged commit 3c7d194 into wasmerio:master Mar 18, 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.

3 participants