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

Make crate no_std compatible #76

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

encounter
Copy link

Notable changes:

  • All tests were wrapped in #[cfg(test)] mod test {}, so that tests can use extern crate std; while building the library itself with no_std. Required to use insta within the test modules. (Disabling whitespace changes in the diff view makes this easier to read!) This is also why the snapshot tests were renamed: they need to include the new test module in the path.
  • udiff's to_writer implementations were wrapped in #[cfg(feature = "std")], given their reliance on std::io.

The cargo test --no-default-features should handle testing the no_std paths in CI already.

Things I'm not sure about:

  • How to handle deadlines in no_std land. Right now, it defines
    #[cfg(not(any(feature = "std", feature = "wasm32_web_time")))]
    pub type Instant = ();
    which I'm not happy with. Maybe the *_deadline functions should be omitted entirely when compiling without a time provider? This is a more involved change, which is why I did not do it in this PR. Other suggestions welcome.
  • hashbrown was added as a dependency because alloc::collections only contains BTreeMap. Rust's std::collections::HashMap requires a random source. With my changes:
    #[cfg(not(feature = "std"))]
    use hashbrown::HashMap;
    #[cfg(feature = "std")]
    use std::collections::HashMap;
    Unfortunately, I don't think there's a way to remove the dependency if compiling with std, so it will always show up in the dependency list, regardless of if it's used. Alternatively, we could use BTreeMap for these instead? But that'd require a breaking API change. (Requiring Idx::Output: PartialOrd rather than Hash.) Again, suggestions appreciated.

@mitsuhiko
Copy link
Owner

How to handle deadlines in no_std land. Right now, it defines

Probably similar to how we are doing with WASM. We define an instant type that is made up and ignored. But yes, it would be better if the entire instant system was just not there if it cannot be provided.

Since adding std as a feature might require a major bump anyways, maybe this is the moment to clean up this entirely and take away the deadline functions if they cannot be provided.

Alternatively, we could use BTreeMap for these instead? But that'd require a breaking API change. (Requiring Idx::Output: PartialOrd rather than Hash.) Again, suggestions appreciated

Since we probably need to bump the major anyways, I think it would be sensible to change the bounds to also require PartialOrd. That leaves options open for alternatives internally.

@encounter
Copy link
Author

I migrated the logic into _internal functions, and cfg'd out the _deadline functions if neither std or wasm32_web_time are enabled.

Regarding HashMap, I'm thinking either:

  • Are we okay with hashbrown existing as a dependency but being unused when std is enabled? It seems to be a very common dependency. Alternatively, it could always be the HashMap, and we ignore std's.
  • If it's really undesired, there could have a separate no_std feature that enables hashbrown. In lib.rs, we can require that either std or no_std is enabled, or both. That way people using the default features won't get the unnecessary dependency, and people who want to disable std can use default-features = false, features = ["no_std"], enabling the hashbrown dependency. I've seen this approach used in a couple of crates.

I was looking at the existing HashMap usages more, and I don't think BTreeMap would be the right thing to use for those.

@mitsuhiko
Copy link
Owner

I think the right course of action for hashbrown is to

  1. use BTreeMap in no_std by default
  2. have a hashbrown feature that switches it into a hashmap.

While I agree that a BTreeMap is not the right storage for either of the cases we have today, it might still make sense to have for when you don't opt in. I definitely do not want hashbrown as a general dependency. But we can also say that you need to opt in as otherwise you do not get any functionality associated with the hash maps which would break quite a bit of the crate.

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.

2 participants