Remove masked_crates from clean::Crate
#82960
Merged
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
Previously, `masked_crates` existed both on `Cache` and on `clean::Crate`. During cache population, the `clean::Crate` version was `take`n and moved to `Cache`. This change removes the version on `clean::Crate` and instead directly mutates `Cache.masked_crates` to initialize it. This has the advantage of avoiding duplication and avoiding unnecessary allocation, as well as making the flow of information through rustdoc less confusing. The one downside I see is that `clean::utils::krate()` now uses the side effect of mutating `DocContext.cache` instead of returning the data directly, but it already mutated the `Cache` for other things (e.g., `deref_trait_did`) so it's not really new behavior. Also, `clean::utils::krate()` is only called once (and is meant to only be called once since it performs expensive and potentially destructive operations) so the mutation shouldn't be an issue.
camelid
added
C-cleanup
Contributor
|
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Member
Author
|
r? @jyn514 |
Member
|
r=me with CI passing. Good catch, thanks :) |
Member
Author
|
@bors r=jyn514 rollup |
Collaborator
|
📌 Commit b7d91b0 has been approved by |
bors
added
S-waiting-on-bors
Member
|
@bors rollup=maybe This could theoretically affect perf - I don't think it's worth marking it as rollup=never, but I don't think it needs to always be in a rollup. |
Collaborator
Collaborator
|
☀️ Test successful - checks-actions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Previously,
masked_cratesexisted both onCacheand onclean::Crate. During cache population, theclean::Crateversion wastaken and moved toCache.This change removes the version on
clean::Crateand instead directlymutates
Cache.masked_cratesto initialize it. This has the advantageof avoiding duplication and avoiding unnecessary allocation, as well as
making the flow of information through rustdoc less confusing.
The one downside I see is that
clean::utils::krate()now uses the sideeffect of mutating
DocContext.cacheinstead of returning the datadirectly, but it already mutated the
Cachefor other things (e.g.,deref_trait_did) so it's not really new behavior. Also,clean::utils::krate()is only called once (and is meant to only becalled once since it performs expensive and potentially destructive
operations) so the mutation shouldn't be an issue.
Follow-up to #82018 (comment).
cc @jyn514