Rename dep node "fingerprints" to distinguish key and value hashes#152751
Rename dep node "fingerprints" to distinguish key and value hashes#152751rust-bors[bot] merged 2 commits intorust-lang:mainfrom
Conversation
|
This comment has been minimized.
This comment has been minimized.
4ab471d to
f686619
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
This is a good distinction to make. I want to check my understanding.
- This PR distinguishes key fingerprints from value fingerprints primarily by changing the names of variables and fields, rather than by types.
- In practice,
PackedFingerprintis usually (always?) used for key fingerprints andFingerprintis usually (always?) used for value fingerprints. KeyFingerprintStyledoes encode key-ness in the type system, because it only applies to key fingerprints.
Is that right? Is it worth pushing further to distinguish key vs. value in the type system? I see that Fingerprint has uses outside of the query system, so maybe that makes it difficult?
| /// previous incremental-compilation session. | ||
| /// | ||
| /// Some nodes don't have a meaningful value hash (e.g. queries with `no_hash`), | ||
| /// so they store a dummy value here instead. |
There was a problem hiding this comment.
Is the dummy value zero?
There was a problem hiding this comment.
In practice I think it is, but I wasn't sure if I should commit to that in this comment.
There was a problem hiding this comment.
I ended up adding (e.g. Fingerprint::ZERO) to give context while also hedging against it not being accurate in the future.
Correct.
Roughly true, but mostly for circumstantial reasons.
Currently it is only used in There are currently no uses of
Yes,
If we were to try this, I would probably just add wrapper newtypes for key and value fingerprints. |
Might be worth doing in a follow-up. r=me after my first two comments above are considered. |
f686619 to
3094a1d
Compare
3094a1d to
b015d57
Compare
@bors r=nnethercote |
Rollup of 20 pull requests Successful merges: - #145399 (Unify wording of resolve error) - #150473 (tail calls: fix copying non-scalar arguments to callee) - #152637 (Add a note about elided lifetime) - #152729 (compiler: Don't mark `SingleUseConsts` MIR pass as "required for soundness") - #152751 (Rename dep node "fingerprints" to distinguish key and value hashes) - #152753 (remove the explicit error for old `rental` versions) - #152758 (Remove ShallowInitBox.) - #151530 (Fix invalid `mut T` suggestion for `&mut T` in missing lifetime error) - #152179 (Add documentation note about signed overflow direction) - #152474 (Implement opt-bisect-limit for MIR) - #152509 (tests/ui/test-attrs: add annotations for reference rules) - #152672 (std: use libc version of `_NSGetArgc`/`_NSGetArgv`) - #152711 (resolve: Disable an assert that no longer holds) - #152725 (Rework explanation of CLI lint level flags) - #152732 (add regression test for 147958) - #152745 (Fix ICE in `suggest_param_env_shadowing` with incompatible args) - #152749 (make `rustc_allow_const_fn_unstable` an actual `rustc_attrs` attribute) - #152756 (Miri: recursive validity: also recurse into Boxes) - #152770 (carryless_mul: mention the base) - #152778 (Update tracking issue number for final_associated_functions)
Rollup merge of #152751 - Zalathar:fingerprint, r=nnethercote Rename dep node "fingerprints" to distinguish key and value hashes In the query system's dependency graph, each node is associated with two *fingerprints*: one that is typically a hash of the query key, and one that is typically a hash of the query's return value when called with that key. Unfortunately, many identifiers and comments fail to clearly distinguish between these two kinds of fingerprint, which have very different roles in dependency tracking. This is a frequent source of confusion. This PR therefore tries to establish a clear distinction between: - **Key fingerprints** that help to uniquely identify a node (along with its `DepKind`), and are typically a hash of the query key - **Value fingerprints** that help to determine whether a node can be marked green (despite having red dependencies), and are typically a hash of the query value There should be no change to compiler behaviour. r? nnethercote (or compiler)
In the query system's dependency graph, each node is associated with two fingerprints: one that is typically a hash of the query key, and one that is typically a hash of the query's return value when called with that key.
Unfortunately, many identifiers and comments fail to clearly distinguish between these two kinds of fingerprint, which have very different roles in dependency tracking. This is a frequent source of confusion.
This PR therefore tries to establish a clear distinction between:
DepKind), and are typically a hash of the query keyThere should be no change to compiler behaviour.
r? nnethercote (or compiler)