Skip to content

Enable use from no_std crates#92

Closed
bebcky wants to merge 1 commit intoindexmap-rs:masterfrom
bebcky:no_std
Closed

Enable use from no_std crates#92
bebcky wants to merge 1 commit intoindexmap-rs:masterfrom
bebcky:no_std

Conversation

@bebcky
Copy link
Copy Markdown

@bebcky bebcky 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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