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

Use hashbrown::HashSet instead of ahash::HashSet #12951

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

mtreinish
Copy link
Member

Summary

In the target_transpiler/mod.rs module we were using ahash::HashSet for a hash set implementation, but the rest of Qiskit has standardized on using hashbrown for the HashSet and HashMap types. Hashbrown uses ahash for it's hashing algorithm but it also provides other advantages. To ensure that hash sets are compatible across the library we should be using the same library for everything. To support this goal, this commit also adds a clippy rule that raises a warning if the std library hashmap or hashset is used, or the versions from ahash. This means with our current dependency set the only allowed hashset types are hashbrown::HashMap/HashSet and indexmap::IndexMap/IndexSet (for where we need to maintain insertion order). Ideally we'd have a rule that forces the use of ahash with IndexMap and IndexSet (see #12935) but I don't think clippy exposes an option to enable something like that.

Details and comments

In the `target_transpiler/mod.rs` module we were using ahash::HashSet
for a hash set implementation, but the rest of Qiskit has standardized
on using hashbrown for the `HashSet` and `HashMap` types. Hashbrown uses
ahash for it's hashing algorithm but it also provides other advantages.
To ensure that hash sets are compatible across the library we should be
using the same library for everything. To support this goal, this commit
also adds a clippy rule that raises a warning if the std library hashmap
or hashset is used, or the versions from ahash. This means with our
current dependency set the only allowed hashset types are
`hashbrown::HashMap`/`HashSet` and `indexmap::IndexMap`/`IndexSet` (for
where we need to maintain insertion order). Ideally we'd have a rule
that forces the use of ahash with `IndexMap` and `IndexSet` (see Qiskit#12935)
but I don't think clippy exposes an option to enable something like that.
@mtreinish mtreinish added Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Aug 13, 2024
@mtreinish mtreinish added this to the 1.3 beta milestone Aug 13, 2024
@mtreinish mtreinish requested a review from a team as a code owner August 13, 2024 15:23
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @kevinhartman
  • @mtreinish

Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

This is sensible to me, there's still a lot of confusion as to which Mapping or Collection structures to use. This should allow us to be more consistent about allowing certain things.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10372610940

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.005%) to 89.66%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 91.73%
crates/qasm2/src/parse.rs 6 96.23%
Totals Coverage Status
Change from base Build 10357074988: 0.005%
Covered Lines: 67392
Relevant Lines: 75164

💛 - Coveralls

@raynelfss raynelfss added this pull request to the merge queue Aug 13, 2024
Merged via the queue into Qiskit:main with commit 8e45a5a Aug 13, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants