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

Enable use from no_std crates #92

Closed
wants to merge 1 commit into from
Closed

Conversation

yaram
Copy link

@yaram yaram commented Apr 28, 2019

This PR adds the ability to use this crate with no_std. It has no breaking changes, using a default std feature with an optional core feature. Since HashMap is only included in libstd, it uses the hashmap_core crate to replace it when compiling with no_std.

It may be cleaner to replace uses of std with core or alloc, rather than creating a fake std. Please let me know if you'd prefer that.

@cuviper
Copy link
Member

cuviper commented Apr 30, 2019

It has no breaking changes

Unfortunately, this is a breaking change, for two reasons:

  1. Features must be additive, in that enabling a feature must not constitute a breaking change. Consider if two crates in a dependency both need indexmap, but one asks for no-default-features (for no_std) and the other uses the default with "std". In this case, changing the "std" feature entirely changes the RandomState type used for the default S parameter, which could break the one that didn't ask for "std".

    • We could work around this by not providing a default S at all for no_std, which also requires gating the new and with_capacity methods.
  2. Someone could already be depending on indexmap with no-default-features, expecting this to work with std, not needing the newly-stabilized alloc. So restricting the current std functionality to a new feature flag also constitutes a breaking change, even if it's a default!

    • I ran into this when I tried to implement no_std for the num crates -- see Yank num 0.1.38 rust-num/num#297. The eventual solution was to do this in a new semver version, but then use the semver trick to have the old version re-export everything from the new with "std" enabled.
    • So we could release indexmap 2.0 where "std" is a default feature, and then a new indexmap 1.0.z which re-export the std-enabled items from 2.0.

So there are problems here, but I think they can be solved. It's a little annoying to need 2.0 though -- do you have a motivating use case for this? If it's just "because we can", I'd say we shouldn't bother...

It may be cleaner to replace uses of std with core or alloc, rather than creating a fake std. Please let me know if you'd prefer that.

Yeah, style-wise I'd prefer to use core directly, and only use cfg alloc for Vec and Box as needed.


pub mod collections {
pub mod hash_map {
pub use hashmap_core::map::*;
Copy link
Member

Choose a reason for hiding this comment

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

I just realized this changes our default RandomState type, normally defined only in std, which is also a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think hashmap_core should be dropped from this PR, because it only adds this, which we don't want.

We'd like to not have a default hasher, that should work, and not have IndexMap::new() at all when we are no-std.

@bluss
Copy link
Member

bluss commented Sep 6, 2019

Someone could already be depending on indexmap with no-default-features, expecting this to work with std, not needing the newly-stabilized alloc.

IMHO we don't need to care about this. Because this crate has no default features to start with, we should not let ourselves be held hostage by those that configure like this. I.e default-features=false has been a working but meaningless configuration until we go live with some default feature.

@cuviper
Copy link
Member

cuviper commented Sep 6, 2019

Well FWIW, I got called out on default-features for num: rust-num/num#297

You could choose to write off such users, but #95 shows how to do it without features.

@bluss
Copy link
Member

bluss commented Sep 6, 2019

Ok! I agree, #95 seems good and you've taken it in the right direction for sure. I just didn't understand the build script stuff (because it was not complete).

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.

3 participants