HashStable for rustc_feature::Features: stop hashing compile-time constant #132076
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM, though I will wait for Mark to weigh in before approving, just in case. |
|
I think my reasoning was that the [bool] list might be the same if we reorder features in the list, but ideally would produce a different hash. However thinking more on it the compiler version etc. should also get hashed in if we care about that so it should be fine (as you point out). To some extent it looks like we've regressed on those goals anyway, we're hashing far more than bools these days: But, still, this seems like a good change to make regardless. @bors r=nnethercote,Mark-Simulacrum |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 24, 2024
Rollup of 10 pull requests Successful merges: - rust-lang#130225 (Rename Receiver -> LegacyReceiver) - rust-lang#131169 (Fix `target_vendor` in QNX Neutrino targets) - rust-lang#131623 (misc cleanups) - rust-lang#131756 (Deeply normalize `TypeTrace` when reporting type error in new solver) - rust-lang#131898 (minor `*dyn` cast cleanup) - rust-lang#131909 (Prevent overflowing enum cast from ICEing) - rust-lang#131930 (Don't allow test revisions that conflict with built in cfgs) - rust-lang#131956 (coverage: Pass coverage mappings to LLVM as separate structs) - rust-lang#132076 (HashStable for rustc_feature::Features: stop hashing compile-time constant) - rust-lang#132088 (Print safety correctly in extern static items) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 24, 2024
Rollup merge of rust-lang#132076 - RalfJung:feature-hashing, r=nnethercote,Mark-Simulacrum HashStable for rustc_feature::Features: stop hashing compile-time constant It seems like back in rust-lang@542bc75 this was added as "hash the boolean value of each lang feature", but then in rust-lang@1487bd6 this got split into first hashing a sequence of `bool`s (representing all the features) and then hashing all the feature names... but the list of feature names is a compile-time constant, so it seems entirely unnecessary to hash them? Cc `@Mark-Simulacrum` who wrote the second of the commits mentioned above. Cc `@nnethercote`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems like back in 542bc75 this was added as "hash the boolean value of each lang feature", but then in 1487bd6 this got split into first hashing a sequence of
bools (representing all the features) and then hashing all the feature names... but the list of feature names is a compile-time constant, so it seems entirely unnecessary to hash them?Cc @Mark-Simulacrum who wrote the second of the commits mentioned above.
Cc @nnethercote