-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rustdoc: Finish transition to computed visibility #90844
Conversation
r? @CraftSpider (rust-highfive has picked a reviewer for you, use r? to override) |
cc #88447 @inquisitivecrystal @jyn514 Is there a reason the places I've updated can't use computed visibility, or should it be okay to change? |
None that I'm aware of. It looks like I just missed these. I wasn't aware of the In any case, while other people are in a better position to confirm that the alternations make sense in context, they look good to me. |
This comment has been minimized.
This comment has been minimized.
I've been trying to reduce the usage of
It's no trouble at all; thanks for starting the process in that PR :) |
Looks like a test is failing in CI... |
Marking as draft since I'll want to squash my commits / edit commit messages after you review. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 9793f5fcae6fc2908c0e69b079ec002b1eef92ea with merge d477b91dd0b64ecd708232320626f4ee548ee264... |
☀️ Try build successful - checks-actions |
Queued d477b91dd0b64ecd708232320626f4ee548ee264 with parent 3e018ce, future comparison URL. |
Finished benchmarking commit (d477b91dd0b64ecd708232320626f4ee548ee264): comparison url. Summary: This change led to very large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
Oh oops - feel free to r=me whenever you're ready. @bors r- |
This change should make the code a bit clearer and easier to change.
This should be the last bit of the transition to computed visibility, rather than syntactic visibility.
* Remove outdated comment * Remove duplicated code * Extract helper function
@bors r=jyn514 rollup=maybe |
📌 Commit 64d6997 has been approved by |
Eh, there's slight rustdoc perf improvements, so I'll use iffy @bors rollup=iffy |
(Pretty sure CI failure is spurious, so not unapproving.) |
⌛ Testing commit 64d6997 with merge 2b6918c43c1c70f857c3b543f2ba1e84e891b66b... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (49d4232): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
This finishes the transition to using computed visibility in rustdoc.