-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
librustdoc
: return impl fmt::Display
in more places instead of writing to strings
#137425
base: master
Are you sure you want to change the base?
librustdoc
: return impl fmt::Display
in more places instead of writing to strings
#137425
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…play-redux, r=<try> `librustdoc`: return `impl fmt::Display` in more places instead of writing to strings Continuation of rust-lang#136784 , another attempt at landing the larger parts of rust-lang#136748 . I'd like to, gradually, make all of the building blocks for rendering docs in `librustdoc` return `impl fmt::Display` instead of returning `Strings`, or receiving a `&mut String` (or `&mut impl fmt::Write`). Another smaller end goal is to be able to get rid of [`write_str`](https://github.com/rust-lang/rust/blob/8dac72bb1d12b2649acd0c190e41524f83da5683/src/librustdoc/html/format.rs#L40-L42). This PR is a large step in that direction. Most of the changes are quite mechanical, and split up into separate commits for easier reviewing (hopefully). I took `print_item` and then started by converting all the functions it called (and their dependencies), and the last commit does the conversion for `print_item` itself. Ignoring whitespace should make reviewing a bit easier. And most importantly, perf run shows pretty good results locally, hopefully CI will also show green 😁 r? `@GuillaumeGomez` , if you feel like it.
Looks good to me, let's see what the benches will tell. |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (d331e3c): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 may lead to changes in compiler perf. @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 773.405s -> 773.68s (0.04%) |
Thanks! @bors r+ rollup=never |
Sorry but can you squash this? 28 commits is a bit excessive. |
@bors r- |
Yeah, sure! One big commit then? Not really sure how to group it, otherwise |
Either one big commit but one commit per file would be also be acceptable unless that makes no sense. |
1015ae9
to
479116e
Compare
@fmease squashed into one commit |
Thanks! @bors r=GuillaumeGomez |
Continuation of #136784 , another attempt at landing the larger parts of #136748 .
I'd like to, gradually, make all of the building blocks for rendering docs in
librustdoc
returnimpl fmt::Display
instead of returningStrings
, or receiving a&mut String
(or&mut impl fmt::Write
). Another smaller end goal is to be able to get rid ofwrite_str
.This PR is a large step in that direction.
Most of the changes are quite mechanical, and split up into separate commits for easier reviewing (hopefully). I took
print_item
and then started by converting all the functions it called (and their dependencies), and the last commit does the conversion forprint_item
itself. Ignoring whitespace should make reviewing a bit easier.And most importantly, perf run shows pretty good results locally, hopefully CI will also show green 😁
r? @GuillaumeGomez , if you feel like it.