feat: remove hashbrown dep
#716
Conversation
|
Thanks for working on this! Since We could simplify by automatically using This would give users the best performance by default while keeping What do you think? |
Peponks9
left a comment
Peponks9
left a comment
There was a problem hiding this comment.
Thanks for the feedback guys. Good poin, I’ll remove the hashmaps feature and tie it directly to std.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few small comments inline.
Also, I tried to resolve merge conflicts, but seems like it broke something. Could you please fix this?
| executable = ["concurrent", "dep:clap", "dep:rand-utils"] | ||
| fuzzing = [] | ||
| hashmaps = ["dep:hashbrown"] | ||
| internal = ["concurrent"] |
There was a problem hiding this comment.
Question: why did we make it so that concurrent doesn't imply std, and internal doesn't imply concurrent? Shouldn't this be more like:
concurrent = ["dep:rayon", "std"]
default = ["concurrent", "std"]
executable = ["concurrent", "dep:clap", "dep:rand-utils"]
fuzzing = []
internal = ["concurrent"]
miden-crypto/src/lib.rs
Outdated
| /// By default, this is an alias for the [`alloc::collections::BTreeMap`]; when the `std` feature is | ||
| /// enabled, this is an alias for the `std::collections::HashMap` (iteration order is not | ||
| /// deterministic in that case). | ||
| #[cfg(feature = "std")] | ||
| pub type Map<K, V> = std::collections::HashMap<K, V>; |
There was a problem hiding this comment.
I think the comment is not exactly correct here: by default, we enable std::collections::HashMap.
miden-crypto/src/lib.rs
Outdated
| /// By default, this is an alias for the [`alloc::collections::BTreeMap`], however, when the | ||
| /// `hashmaps` feature is enabled, this is an alias for the `hashbrown`'s `HashMap`. | ||
| #[cfg(not(feature = "hashmaps"))] | ||
| /// `std` feature is enabled, this is an alias for the `std::collections::HashMap`. | ||
| #[cfg(not(feature = "std"))] | ||
| pub type Map<K, V> = alloc::collections::BTreeMap<K, V>; |
There was a problem hiding this comment.
Same comment as above: by default we enable std::collections::HashMap.
This is also relevant in a couple other places in this file.
|
@Peponks9 - are you still interested in wrapping up this PR? |
Peponks9
force-pushed
the
remove-hashbrown
branch
from
4245936 to
007942a
Compare
|
@Peponks9 - after recent merges, there are now quite a few conflicts here - sorry about that! Would you be able to resolve them? |
4 participants
Closes #696