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

2018 edition, no_std and use hashbrown #20

Closed
wants to merge 1 commit into from

Conversation

dingelish
Copy link

@dingelish dingelish commented Mar 8, 2019

Changes:

(1) Migrate to the 2018 edition.
(2) Turns this crate into a no_std crate. It'll benefit a lot of target platforms without a full functional libstd.
(3) Use hashbrown, a high-performance SwissTable hash map (no_std), to replace the one in libstd.

Signed-off-by: Yu Ding dingelish@gmail.com


This change is Reviewable

Signed-off-by: Yu Ding <dingelish@gmail.com>
@SimonSapin
Copy link
Member

It looks like switching to the 2018 edition has no impact at all on this crate. It also has no impact on dependencies, you can mix crates of different editions in the same program.

Just providing a type alias does not feel worth taking on a public dependency: whenever hashbrown 0.2 or 1.0 is released and people start to use it, they won’t want to also have 0.1 in their dependency graph. So this crate would need to be updated, with a semver-breaking change to its own version number. So I’d rather not make that change. Note that you can already use hashbrown::HashMap<K, V, FnvBuildHasher> or define your own type alias for it, without changing this crate.

This leaves no_std. Either:

  • We make it conditional with an std Cargo feature that is enabled by default. The downside is that every single use has to opt out of default features in order for it to be disabled, so default features are somewhat useless.
  • Or we make no_std the default or only mode. (With the type alias being conditional or removed.) I think I’d prefer that over the above, but this is a breaking change so we’d have to do the version number dance and then go through everything in Servo’s dependency graph that uses fnv to make them use the new version. Which I’d rather avoid if there isn’t a good reason.

Is no_std something you actually need in a specific project, or was it a "why not" in passing?

@dingelish
Copy link
Author

Hi Simon, thanks for your explanation.

I'm working on rust-sgx-sdk and working closely with several blockchain projects. I noticed that many of the popular crates used in blockchain projects depend on rust-fnv and people would like to see it could be used in a no_std environment directly. And this is the reason I try to make it no_std and use hashbrown to get rid of libstd.

Your concerns make sense. I do agree with you. For no_std support, I would say it'll be better to have it, but still acceptable if not. I could maintain a fork with some minor changes and enable auto-merge from the upstream. What do you think? :-)

@SimonSapin
Copy link
Member

I fork might not really help you with your dependencies that depend on fnv, unless you convince them to switching to using your fork. At that point it’s better for everyone if you instead ask them to update to fnv 2.0 which is no_std and doesn’t define type aliases for HashMap or HashSet. (Or does with an off-by-default feature flag, but idk if it’s even worth to bother. CC @cuviper who added them in #12.)

@dingelish dingelish closed this Mar 8, 2019
@cuviper
Copy link
Contributor

cuviper commented Mar 8, 2019

We make it conditional with an std Cargo feature that is enabled by default.

Note, this is still a breaking change, as it would remove functionality from the featureless crate. There's no real reason anyone should use no-default-features now, but they could, and that would break.

You could manage the change with a server bump and a semver-trick re-export in the old, which is what I did to add no_std in num-traits. That may be overkill here though.

@SimonSapin
Copy link
Member

@cuviper I see. With off-by-default in fnv 2.0 then, do you think it’s worth having type aliases at all?

@dingelish Did you mean to close this? Sorry if I came across as opposed to any change, I think making fnv no_std is good but there’s some details to figure out.

@cuviper
Copy link
Contributor

cuviper commented Mar 8, 2019

I would still like to have the aliases in some way, even if I have to opt in to a feature. The types are cumbersome to type otherwise.

IMO, it's fine to keep std in the default feature set though. Every crate that wants to be no_std already has to make this a conscious choice, and I think it's already common that this requires no-default-features on most dependencies.

@dingelish
Copy link
Author

@SimonSapin I think that another PR on no_std support would be better :-)

@SimonSapin
Copy link
Member

@cuviper I agree that no type alias at all is cumbersome. What would you think of having type alisases in documentation that you can copy/paste into your crates?

@cuviper
Copy link
Contributor

cuviper commented Mar 10, 2019

I was thinking that you meant all of the aliases would be going away, but I suppose you can at least keep FnvBuildHasher with no_std. The map/set aliases are not too bad from there, but yes, it would be fitting to at least document this.

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.

None yet

3 participants