rustdoc: Cleanup ExternalCrate#86889
Conversation
jyn514
commented
Jul 5, 2021
jyn514
commented
- Remove unnecessary CrateNum from Cache.externs
- Remove trival impl Clean for CrateNum
jyn514
added
C-cleanup
|
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @camelid |
camelid
left a comment
camelid
left a comment
There was a problem hiding this comment.
Thanks, this is great! I have two comments:
src/librustdoc/clean/utils.rs
Outdated
There was a problem hiding this comment.
I'm wondering if maybe it would be better to sort_unstable_by_key(|c| c.crate_num) instead (and then remove the Ord derive from ExternalCrate), to make it more explicit what the crates are being sorted by. Also, it would the code more resilient to change in ExternalCrate. But I'm okay with leaving it how it is now if that's what you'd prefer. What do you think?
There was a problem hiding this comment.
Actually, on second thought, why do we sort the externs at all? Is it to prevent non-determinism? Can you try switching it to use BTreeSet instead? (If you don't want to do it now, that's okay, but I wanted to mention it.)
There was a problem hiding this comment.
The order is used here: https://github.com/rust-lang/rust/blob/23fdc18eb68b1ec5daee2d095441d47afb0ce89e/src/librustdoc/formats/cache.rs#L161-L165
I have no idea why, the comment doesn't make much sense to me since the crate number has very little relation to the dependency depth it's at. I would be fine with removing it.
There was a problem hiding this comment.
Oh! This would be a great way to handle #73423, we can have different docs in core than in std. I didn't realize rustdoc silently overwrote the old docs.
There was a problem hiding this comment.
It turns out the sort can't be removed :/ it would cause the docs in core to take precedence over those defined in downstream crates. That wouldn't be a problem, except that doc(primitive) is stable for some reason and it would be a change in observable behavior.
I'll add back the sorting. I still think this is the right approach for fixing 73423 though.
camelid
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
crate-cleanup
branch
2 times, most recently
from
23fdc18 to
cbfad16
Compare
jyn514
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It can be found from ExternalCrate.
|
I will hopefully be able to review this in the next few days. |
|
Thanks! @bors r+ rollup |
|
📌 Commit a30fa08 has been approved by |
bors
added
S-waiting-on-bors
rustdoc: Cleanup ExternalCrate - Remove unnecessary CrateNum from Cache.externs - Remove trival impl Clean for CrateNum
Rollup of 8 pull requests Successful merges: - rust-lang#86763 (Add a regression test for issue-63355) - rust-lang#86814 (Recover from a misplaced inner doc comment) - rust-lang#86843 (Check that const parameters of trait methods have compatible types) - rust-lang#86889 (rustdoc: Cleanup ExternalCrate) - rust-lang#87092 (Remove nondeterminism in multiple-definitions test) - rust-lang#87170 (Add diagnostic items for Clippy) - rust-lang#87183 (fix typo in compile_fail doctest) - rust-lang#87205 (rustc_middle: remove redundant clone) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Now that rust-lang#73423 is fixed, sorting should no longer be necessary. See also this discussion [1]. [1]: rust-lang#86889 (comment)
7 participants
