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

Possible changes for 2.0 #135

Closed
3 of 7 tasks
cuviper opened this issue Jul 2, 2020 · 6 comments · Fixed by #267
Closed
3 of 7 tasks

Possible changes for 2.0 #135

cuviper opened this issue Jul 2, 2020 · 6 comments · Fixed by #267

Comments

@cuviper
Copy link
Member

cuviper commented Jul 2, 2020

Let's use this issue to discuss changes for a hypothetical 2.0.

Proposed

  • Increase the MSRV from 1.18.
    • This is not strictly breaking, as we did state the intent to allow increases in minor releases, but in 2.0 we can do this without any fear of harming our dependents.
      • In particular, serde_json supports Rust 1.31.
    • Implement index-sliced key-value iterators #104 wants 1.28 for RangeBounds.
    • Switch to hashbrown's RawTable internally #131 wants 1.32 for hashbrown.
    • 1.34 adds TryFrom/TryInto which may be useful for custom indexing.
    • With 1.36, we could assume alloc support.
    • DONE: We went ahead with MSRV 1.32 in indexmap 1.5, and MSRV 1.36 in 1.6
  • Change the default hasher to something faster.
    • e.g. hashbrown defaults to ahash.
    • Especially with MSRV 1.36, a no_std default would be nice.
    • We can wrap it to avoid committing to a specific public dependency.
  • Parameterize the index type.
    • While we could try to default Idx = usize, this doesn't always work well with type inference.
  • Maybe change the default remove()?
    • (bluss) I still think the current decision is fine, no change from my end for 2.0. (Good way to document this would be to say that "remove is the fastest way to remove an element, and if order should be preserved, ...")
  • std crate feature
  • Consider ordered PartialEq + Eq, more like Vec than HashMap (IndexMap / IndexSet comparison isn't order-aware. #153, map: Make Eq comparisons ordering-aware. #154).
    • This would also allow PartialOrd, Ord, and Hash.
  • get_index_mut should return &K instead of &mut K (Add more Vec/slice-like methods to maps and sets #160 (comment))
@bluss
Copy link
Member

bluss commented Jul 3, 2020

Added item for a proper std crate feature. Makes it easier for our rdeps

@cuviper
Copy link
Member Author

cuviper commented Jul 5, 2020

If we adopt MSRV 1.36 and a new default hasher, I think there will be no more need for std at all. But it's not certain that we should do that.

@bluss
Copy link
Member

bluss commented Jul 10, 2020

I just have an attitude problem with ahash, would be more charmed by a pure-rust hash function.

But for an actual argument, for a public dependency we need them to have an 1.x version or equivalent.

@ammkrn
Copy link

ammkrn commented Mar 9, 2022

Are there any plans to support the Allocator trait/allocator_api feature? hashbrown::raw::RawTable and std::Vec both expose the extra A : Allocator type parameter, so it seems possible.

@cuviper
Copy link
Member Author

cuviper commented Mar 9, 2022

I've thought about Allocator, but it's difficult to predict when that will even be stable in the standard library. You can't really do anything with that on Vec until it's stable, while hashbrown sort of cheats by defining their own private copy when the nightly feature isn't enabled.

(I'm generally not inclined to use unstable features in this crate, not even behind a feature flag.)

@ammkrn
Copy link

ammkrn commented Mar 9, 2022

Thanks for taking a minute to explain the issue. I have a use case for this, but I'm with you on not wanting to support unstable features, so I'm happy if it's on the "probably at some point after stabilization if we feel like it" list.

@cuviper cuviper pinned this issue May 24, 2022
@cuviper cuviper linked a pull request Jun 23, 2023 that will close this issue
@cuviper cuviper unpinned this issue Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants