Pass Context as &mut in rustdoc
#91468
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
src/librustdoc/html/render/mod.rs
Outdated
| ) { | ||
| write!( | ||
| w, | ||
| "{}{}const <a href=\"{}\" class=\"constant\">{}</a>: {}", | ||
| extra, | ||
| it.visibility.print_with_space(it.def_id, cx), | ||
| it.visibility.print_with_space(it.def_id, &cx.clone()), |
There was a problem hiding this comment.
Why is this clone necessary? I think you can just do &*cx.
There was a problem hiding this comment.
Not sure, let me test
There was a problem hiding this comment.
Ah right, it differs the mutability and causes borrowing issues.
There was a problem hiding this comment.
What about moving this up and before the write so you can use &mut cx after?
|
r? @jyn514 |
Co-authored-by: Joshua Nelson <[email protected]>
jyn514
added
C-cleanup
This comment has been minimized.
This comment has been minimized.
|
@Milo123459 I suspect the test failure is because of all the new clones you added and that rustdoc depended on the state being shared. Do you think it would be feasible to remove the clones? I can help you out if you run into trouble :) |
|
Sure! Thanks |
jyn514
added
S-waiting-on-author
| @@ -43,6 +43,7 @@ use crate::try_err; | |||
| /// It is intended that this context is a lightweight object which can be fairly | |||
| /// easily cloned because it is cloned per work-job (about once per item in the | |||
| /// rustdoc tree). | |||
| #[derive(Clone)] | |||
There was a problem hiding this comment.
This must be removed before this PR is merged; Context intentionally does not implement Clone (cf. make_child_renderer()). We should probably add something to its docs about this.
There was a problem hiding this comment.
Alright, shall work on it.
There was a problem hiding this comment.
Please feel free to ask for help here or on Zulip :)
There was a problem hiding this comment.
An easy way to check that a type doesn't implement a trait is to have a doc code like this:
/// ```compile_fail
/// let c = Context { ... };
/// c.clone();
/// ```
:)
There was a problem hiding this comment.
Hey, are you suggesting that has been implemented or should be implemented?
Anyway - I'm almost finished removing the clones!
There was a problem hiding this comment.
Yea I take that back, this is proving to be a challenge haha
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #91957) made this pull request unmergeable. Please resolve the merge conflicts. |
Solves #90323