Skip to content

feat: add no_std macros indexmap_with_default and indexset_with_default #380

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

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

iajoiner
Copy link
Contributor

@iajoiner iajoiner commented Feb 26, 2025

Closes #379.
I'm adding indexmap_with_default and indexset_with_default which should be no-std.

Thanks @JayWhite2357 for providing a solution in Proof of SQL which we are upstreaming!

@cuviper
Copy link
Member

cuviper commented Feb 26, 2025

This will break type inference, because the compiler can't tell which S hasher to use, and the default doesn't apply when used like this. I'll bet it's fine in your local version because you've type-aliased them with ahash.

src/macros.rs Outdated
@@ -63,7 +60,8 @@ macro_rules! indexset {
// Note: `stringify!($value)` is just here to consume the repetition,
// but we throw away that string literal during constant evaluation.
const CAP: usize = <[()]>::len(&[$({ stringify!($value); }),*]);
let mut set = $crate::IndexSet::with_capacity(CAP);
#[allow(unused_mut)]
let mut set = $crate::IndexSet::with_capacity_with_hasher(CAP, <_>::default());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut set = $crate::IndexSet::with_capacity_with_hasher(CAP, <_>::default());
let mut set = $crate::IndexSet::with_capacity_and_hasher(CAP, <_>::default());

@iajoiner
Copy link
Contributor Author

iajoiner commented Feb 26, 2025

@cuviper Really thanks for letting me know! How do you think we can modify it to fix the issue then? I don't really want this to depend on any particular hasher. Maybe the hasher should be made easy to specify somehow?

@iajoiner
Copy link
Contributor Author

Will something like this be acceptable?

#[macro_export]
macro_rules! indexmap {
    ($S:ty; $($key:expr => $value:expr),*) => {{
        const CAP: usize = <[()]>::len(&[$({ stringify!($key); }),*]);
        #[allow(unused_mut)]
        // Specify your custom S (must implement Default) as the hasher:
        let mut map = $crate::IndexMap::<_, _, $S>::with_capacity_and_hasher(CAP, <$S>::default());

        $(
            map.insert($key, $value);
        )*

        map
    }};
    // fallback without hasher type specified
    ($($key:expr => $value:expr),*) => {{
        use std::collections::hash_map::RandomState;
        const CAP: usize = <[()]>::len(&[$({ stringify!($key); }),*]);
        #[allow(unused_mut)]
        let mut map = $crate::IndexMap::<_, _, RandomState>::with_capacity_and_hasher(
            CAP, 
            RandomState::default()
        );

        $(
            map.insert($key, $value);
        )*

        map
    }};
}

This way if std is enabled we can allow the old macro without any change. Otherwise we require the hasher to be specified.

@cuviper
Copy link
Member

cuviper commented Feb 26, 2025

I don't know -- this is why the macros are currently hard-coded. 😉

FromIterator does have open S, and now that this trait is in the prelude, it will work reasonably well with array iterators:

// this still needs *something* to drive the type inference for `S`
let map = IndexMap::from_iter([(1, 'a'), (2, 'b'), (3, 'c')]);

@cuviper
Copy link
Member

cuviper commented Feb 26, 2025

Will something like this be acceptable?

#[macro_export]
macro_rules! indexmap {
    ($S:ty; $($key:expr => $value:expr),*) => {{
        [...]
    }};
    // fallback without hasher type specified
    ($($key:expr => $value:expr),*) => {{
        [...]
    }};
}

If this works without any change to the existing tests, then yes, I think we can do that!

@JayWhite2357
Copy link

Another solution could be this:

#[cfg(feature = "std")]
let mut map = $crate::IndexMap::with_capacity(CAP);
#[cfg(not(feature = "std"))]
let mut map = $crate::IndexMap::with_capacity_and_hasher(CAP, <_>::default());

My syntax might be off, but something like that should work. It would require specifying the type explicitly when std is off, but it would be essentially identical semantics otherwise. Very similar to the from_iter.

@cuviper
Copy link
Member

cuviper commented Feb 26, 2025

No, we can't just alternate it because indexmap could be a dependency of multiple parts of the build tree, perhaps with and without the "std" feature enabled, so it will be unified to enabled. Then the crate which wanted it with some other hasher will be broken.

@JayWhite2357
Copy link

No, we can't just alternate it because indexmap could be a dependency of multiple parts of the build tree, perhaps with and without the "std" feature enabled, so it will be unified to enabled. Then the crate which wanted it with some other hasher will be broken.

Ah, yes.

@iajoiner
Copy link
Contributor Author

iajoiner commented Feb 26, 2025

@cuviper @JayWhite2357 Alternatively maybe we can separate indexmap_with_hasher / indexset_with_hasher from indexmap and indexset. Then users who have picked a particular hasher can just define their own indexmap and indexset using indexmap_with_hasher / indexset_with_hasher.

I do not want to define indexmap and indexset using the new macros with hasher yet to make sure they actually work well without breaking downstream code.

@iajoiner iajoiner marked this pull request as draft February 26, 2025 02:34
@iajoiner iajoiner changed the title feat: make indexmap and indexset no-std feat: add no_std macros indexmap_with_hasher and indexset_with_hasher Feb 26, 2025
@iajoiner iajoiner force-pushed the feat/macros-no-std branch 2 times, most recently from 25d45d4 to c94b22b Compare February 26, 2025 02:43
@iajoiner iajoiner marked this pull request as ready for review February 26, 2025 02:50
@iajoiner iajoiner force-pushed the feat/macros-no-std branch 2 times, most recently from ff6c67a to d1784ac Compare February 26, 2025 03:07
@iajoiner iajoiner requested a review from cuviper February 26, 2025 03:17
@iajoiner
Copy link
Contributor Author

iajoiner commented Feb 26, 2025

Now use cases need to look like this if we use a different hasher

use ahash::AHasher;
use indexmap::indexmap_with_hasher;

fn main() {
    let map = indexmap_with_hasher! {
        AHasher; "a" => 1, "b" => 2,
    };
    assert_eq!(map["a"], 1);
    assert_eq!(map["b"], 2);
    assert_eq!(map.get("c"), None);
}

Alternatively people annoyed about having to specify the hasher all the time can do

use ahash::AHasher;
use indexmap::indexmap_with_hasher;

macro_rules! indexmap {
    ($($key:expr => $value:expr,)+) => { indexmap_with_hasher!{AHasher; $($key => $value),+} };
    ($($key:expr => $value:expr),*) => { indexmap_with_hasher!{AHasher; $($key => $value),*} };
}

fn main() {
    let map = indexmap! {
        "a" => 1, "b" => 2,
    };
    assert_eq!(map["a"], 1);
    assert_eq!(map["b"], 2);
    assert_eq!(map.get("c"), None);
}

@iajoiner iajoiner force-pushed the feat/macros-no-std branch 2 times, most recently from 62a81c4 to 2df8725 Compare February 26, 2025 03:20
@iajoiner
Copy link
Contributor Author

I have tried this with both ahash and twox-hash and DefaultHasher is covered in doc tests so I think we are good to go.

@iajoiner iajoiner force-pushed the feat/macros-no-std branch from 2df8725 to 877b133 Compare March 3, 2025 16:42
@iajoiner iajoiner requested a review from cuviper March 3, 2025 16:44
@iajoiner iajoiner changed the title feat: add no_std macros indexmap_with_hasher and indexset_with_hasher feat: add no_std macros indexmap_with_default and indexset_with_default Mar 3, 2025
@iajoiner iajoiner force-pushed the feat/macros-no-std branch from 877b133 to 6301a2a Compare March 3, 2025 16:46
@iajoiner iajoiner force-pushed the feat/macros-no-std branch from 6301a2a to 941caf4 Compare March 3, 2025 21:17
@iajoiner iajoiner requested a review from cuviper March 3, 2025 21:17
@iajoiner iajoiner force-pushed the feat/macros-no-std branch from 941caf4 to 5fd8971 Compare March 5, 2025 01:12
@iajoiner
Copy link
Contributor Author

iajoiner commented Mar 5, 2025

@cuviper Really thanks! It's ready again haha.

@iajoiner iajoiner requested a review from cuviper March 5, 2025 01:13
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@cuviper cuviper added this pull request to the merge queue Mar 5, 2025
Merged via the queue into indexmap-rs:main with commit b10b273 Mar 5, 2025
15 checks passed
@iajoiner iajoiner deleted the feat/macros-no-std branch March 5, 2025 04:15
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.

We need indexmap and indexset-like macros that don't require std
3 participants