KGrewal1 opened PR #8498 from KGrewal1:fxhash-to-rustchash
to bytecodealliance:main
:
This removes the internal code for fxhash, and subsequently migrates use of the fxhash crate (https://crates.io/crates/fxhash) to the rustc-hash crate (https://crates.io/crates/rustc-hash) implementing the same hash function: I believe the latter will need to be audited however--- @fitzgen
KGrewal1 requested elliottt for a review on PR #8498.
KGrewal1 requested wasmtime-compiler-reviewers for a review on PR #8498.
KGrewal1 requested wasmtime-default-reviewers for a review on PR #8498.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
There is no need to rename this package, right?
KGrewal1 updated PR #8498.
KGrewal1 submitted PR review.
KGrewal1 created PR review comment:
I actually made a mistake: fxhash specifically is currently needed as souper-harvest needs this hash convenience function which is not present in rustc-hash https://docs.rs/fxhash/0.2.1/fxhash/fn.hash.html .
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I didn't mean undoing the migration, I mean using
rustc-hash = "1.1.0"
instead offxhash = {package = "rustc-hash", version = "1.1.0"}
.
KGrewal1 submitted PR review.
KGrewal1 created PR review comment:
I chose to rename though to minimise the size of the diff: appreciate if that is considered ugly and so not done
KGrewal1 edited PR #8498:
This removes the internal code for fxhash, <s>and subsequently migrates use of the fxhash crate (https://crates.io/crates/fxhash) to the rustc-hash crate (https://crates.io/crates/rustc-hash) implementing the same hash function: I believe the latter will need to be audited however<\s> --- @fitzgen
KGrewal1 edited PR #8498:
This removes the internal code for fxhash and instead uses the fxhash crate <s>and subsequently migrates use of the fxhash crate (https://crates.io/crates/fxhash) to the rustc-hash crate (https://crates.io/crates/rustc-hash) implementing the same hash function: I believe the latter will need to be audited however<\s> --- @fitzgen
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I actually made a mistake: fxhash specifically is currently needed as souper-harvest needs this hash convenience function which is not present in rustc-hash
Inlining the convenience function is fine I think.
I chose to rename though to minimise the size of the diff: appreciate if that is considered ugly and so not done
Doing the move in a separate commit without renaming would still be easy to review.
KGrewal1 updated PR #8498.
KGrewal1 edited PR #8498:
This removes the internal code for fxhash and instead uses the fxhash crate and subsequently migrates use of the fxhash crate (https://crates.io/crates/fxhash) to the rustc-hash crate (https://crates.io/crates/rustc-hash) implementing the same hash function: I believe the latter will need to be audited however: also manually implement the hash convenience function from fxhash --- @fitzgen
KGrewal1 submitted PR review.
KGrewal1 created PR review comment:
Have now done this
fitzgen submitted PR review:
Thanks! I'll do the audit and merge this as well in another PR. Will link to it when it is ready.
fitzgen commented on PR #8498:
Ah, looks like the Firefox folks kindly already audited
rustc-hash
so we can just go ahead and merge this!
fitzgen merged PR #8498.
scottmcm submitted PR review.
scottmcm created PR review comment:
If your MSRV is at least 1.71, you might be interested in <https://doc.rust-lang.org/std/hash/trait.BuildHasher.html#method.hash_one> for this, though that gives
u64
instead ofusize
.
KGrewal1 submitted PR review.
KGrewal1 created PR review comment:
Looking at the code I think that would work (and MSRV is 1.75)? Can't see any reason that it'd need to be usize, if anyone else knows more about it than me?
KGrewal1 edited PR review comment.
fitzgen submitted PR review.
fitzgen created PR review comment:
u64
would indeed be fine.
scottmcm submitted PR review.
scottmcm created PR review comment:
To help avoid this problem in future I've gone and added an
FxBuildHasher
to rustc-hash,I'll try to remember to come back here and simplify #8677 once there's a new release of rustc-hash on crates-dot-io to use.
Last updated: Dec 23 2024 at 13:07 UTC