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.

Sign up for GitHub

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

Unify std and no_std APIs for IndexMap and IndexSet #207

Closed
wants to merge 1 commit into from
Closed

Unify std and no_std APIs for IndexMap and IndexSet #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ rayon = { version = "1.2", optional = true }

[dependencies.hashbrown]
version = "0.11"
default-features = false
features = ["raw"]
features = ["ahash", "raw"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahash pulls in a non-trivial amount of dependencies. On linux it pulls in libc, version_check, cfg-if, once_cell and getrandom.

Copy link
Author

Choose a reason for hiding this comment

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

Good find, I hadn't noticed that since I primarily build for a custom no_std target. If you're aware of other hashing crates with fewer dependencies, I could experiment with using them as the default. I'll look for others myself too, in the meantime.

If considering multiple hashing crates, we could gate the inclusion of each one behind a feature; this would allow std and no_std users alike to select different default hashers.

Copy link
Member

Choose a reason for hiding this comment

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

We can only do that if we make the features mutually exclusive, as a hard build error. You could have one part of a large dependency tree that wants indexmap one way, and another part wanting the other way, but Cargo unifies dependencies into one build so we can't satisfy both.

But that's only about default S and the few methods that assume that. Most of the API is perfectly fine with bring-your-own-hasher S, so each part of that dependency tree can fill their own needs.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I had used that approach of pseudo-mutually-exclusive features for another project via the compile_error!() macro, but it's admittedly tedious and feels hacky.

I do hear what you're saying about multiple downstream crates using indexmap differently, and unfortunately there's no way around that.


[dev-dependencies]
itertools = "0.9"
Expand Down
10 changes: 6 additions & 4 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,13 @@
//! trigger this. It can be tested by building for a std-less target.
//!
//! - Creating maps and sets using [`new`][IndexMap::new] and
//! [`with_capacity`][IndexMap::with_capacity] is unavailable without `std`.
//! Use methods [`IndexMap::default`][def],
//! [`with_capacity`][IndexMap::with_capacity] are available without `std`;
//! they use [`hashbrown::hash_map::DefaultHashBuilder`] as their default
//! hash builder.
//! You can also use methods [`IndexMap::default`][def],
//! [`with_hasher`][IndexMap::with_hasher],
//! [`with_capacity_and_hasher`][IndexMap::with_capacity_and_hasher] instead.
//! A no-std compatible hasher will be needed as well, for example
//! [`with_capacity_and_hasher`][IndexMap::with_capacity_and_hasher],
//! optionally with other no_std-compatible hashers, for example
//! from the crate `twox-hash`.
//! - Macros [`indexmap!`] and [`indexset!`] are unavailable without `std`.
//!
Expand Down
5 changes: 3 additions & 2 deletions src/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use ::core::slice::{Iter as SliceIter, IterMut as SliceIterMut};

#[cfg(has_std)]
use std::collections::hash_map::RandomState;
#[cfg(not(has_std))]
use hashbrown::hash_map::DefaultHashBuilder;

use self::core::IndexMapCore;
use crate::equivalent::Equivalent;
Expand Down Expand Up @@ -73,7 +75,7 @@ pub struct IndexMap<K, V, S = RandomState> {
hash_builder: S,
}
#[cfg(not(has_std))]
pub struct IndexMap<K, V, S> {
pub struct IndexMap<K, V, S = DefaultHashBuilder> {
core: IndexMapCore<K, V>,
hash_builder: S,
}
Expand Down Expand Up @@ -140,7 +142,6 @@ where
}
}

#[cfg(has_std)]
impl<K, V> IndexMap<K, V> {
/// Create a new map. (Does not allocate.)
#[inline]
Expand Down
5 changes: 3 additions & 2 deletions src/set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ pub use crate::rayon::set as rayon;

#[cfg(has_std)]
use std::collections::hash_map::RandomState;
#[cfg(not(has_std))]
use hashbrown::hash_map::DefaultHashBuilder;
Copy link
Member

Choose a reason for hiding this comment

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

As @cuviper said, we can't expose hashbrown types in our public API. This change would make hashbrown a public dependency and it would not be possible for us to bump that version without changing our own major version. As cuviper said, we could just solve that with a wrapper.

Hashbrown is not becoming our public dependency, because it causes problems for our own versioning story.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, thanks for the clarification.


use crate::vec::{self, Vec};
use core::cmp::Ordering;
Expand Down Expand Up @@ -64,7 +66,7 @@ pub struct IndexSet<T, S = RandomState> {
map: IndexMap<T, (), S>,
}
#[cfg(not(has_std))]
pub struct IndexSet<T, S> {
pub struct IndexSet<T, S = DefaultHashBuilder> {
map: IndexMap<T, (), S>,
}

Expand Down Expand Up @@ -124,7 +126,6 @@ where
}
}

#[cfg(has_std)]
impl<T> IndexSet<T> {
/// Create a new set. (Does not allocate.)
pub fn new() -> Self {
Expand Down