Make crate no_std compatible
Notable changes:
- All tests were wrapped in
#[cfg(test)] mod test {}, so that tests can useextern crate std;while building the library itself withno_std. Required to useinstawithin 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 newtestmodule in the path. udiff'sto_writerimplementations were wrapped in#[cfg(feature = "std")], given their reliance onstd::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_stdland. Right now, it defines
which I'm not happy with. Maybe the#[cfg(not(any(feature = "std", feature = "wasm32_web_time")))] pub type Instant = ();*_deadlinefunctions 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. hashbrownwas added as a dependency becausealloc::collectionsonly containsBTreeMap. Rust'sstd::collections::HashMaprequires a random source. With my changes:
Unfortunately, I don't think there's a way to remove the dependency if compiling with#[cfg(not(feature = "std"))] use hashbrown::HashMap; #[cfg(feature = "std")] use std::collections::HashMap;std, so it will always show up in the dependency list, regardless of if it's used. Alternatively, we could useBTreeMapfor these instead? But that'd require a breaking API change. (RequiringIdx::Output: PartialOrdrather thanHash.) Again, suggestions appreciated.
How to handle deadlines in
no_stdland. 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: PartialOrdrather 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.
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
stdis 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_stdfeature that enableshashbrown. Inlib.rs, we can require that eitherstdorno_stdis enabled, or both. That way people using the default features won't get the unnecessary dependency, and people who want to disablestdcan usedefault-features = false, features = ["no_std"], enabling thehashbrowndependency. 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.
I think the right course of action for hashbrown is to
- use
BTreeMapinno_stdby default - have a
hashbrownfeature 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.
Sorry for the delay. I implemented the suggestions:
default-features = false->alloc::collections::BTreeMapdefault-features = false, features = ["hashbrown"]->hashbrown::HashMapdefault-features = trueorfeatures = ["std"]->std::collections::HashMap
This required adding the Ord bound in a few places. Thankfully, the main diff/diff_deadline functions already had an Ord bound, so I doubt this will cause issues.
@mitsuhiko Are you still interested in these changes? Happy to make further modifications if necessary
Hi @encounter , I also have a change I'd like to make with respect to floating point diffs.
Would you like to work on a fork together? Not necessarily to actively maintain it, but have a new established repository with a few requested features.
Let me know! I couldn't find your contact on your website, shoot me an email at [email protected].
@mitsuhiko sorry to nag, but I'm also looking for this feature in my project and am really hoping it can be merged upstream if it's acceptable to you. If there's anything I can do to help also, I'm happy to. thanks for your time and energy on this
@ethteck @encounter I have made a fork here and will soon publish the package to the crates.io registry with a new name (thinking likewise) right now.
I don't have much experience but would appreciate y'all migrating your prs or helping it (if you'd like!) in this port.