librustdoc: remove unused for_html field from Buffer 🦄
#136656
Closed
Conversation
This file contains 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
rustbot
added
S-waiting-on-review
|
I think we can get rid of |
|
Cool! On it. |
|
Superseded by #136748 |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 9, 2025
…r=<try> Nuke `Buffer` abstraction from `librustdoc` 💣 In rust-lang#136656 I found out that the `for_html` field in the `Buffer` struct was never read, and pondered if `Buffer` had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@GuillaumeGomez` But... this got *a lot* bigger than I had planned. A lot of functions had a `&mut Buffer` arg, but instead of replacing it with a `buf: impl fmt::Write` arg, I decided to change those functions to return an opaque `impl fmt::Display` instead. If this PR turns out to be contentious I can try to make a PR that just removes the `Buffer` struct and tries to make less invasive changes, but personally I do like some of the cleanups that this PR allows. Let's see what others think! I think it'll be better to review this without whitespace (If this gets positive reactions, I'll need to rebase and maybe try to separate this into logical commits, but not sure if that's very practical) While most of the PR is "cosmetic", I did make some small changes, mostly trying to make some of the formatting lazier, and do less allocations. So a perf run will be nice :) ### Pros and cons of returning `impl fmt::Display` instead of taking a `impl fmt::Write` #### Cons: - Named lifetimes: function signatures got a lot more verbose because the RPIT opaque type needs to be explicitly bound by the lifetimes of the refs in the arguments - Having to use `fmt::from_fn` causes another level of indentation - Immutable closures, can't move out of non-`Copy` items (wasn't much of a problem in practice) #### Pros: - Less arguments, no un-Rusty "out" argument - Nicer composability, allows the returned type to be directly used in format strings ### Interchangeability A function receiving a `impl fmt::Write` can be turned into a function returning a `impl fmt::Display` by using `fmt::from_fn`.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 9, 2025
…ake2, r=<try> Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Feb 12, 2025
…-take2, r=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Feb 12, 2025
…-take2, r=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 12, 2025
Rollup merge of rust-lang#136784 - yotamofek:pr/rustdoc-remove-buffer-take2, r=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes rust-lang#136748
github-actions bot
pushed a commit
to rust-lang/rustc-dev-guide
that referenced
this pull request
Feb 13, 2025
…=GuillaumeGomez Nuke `Buffer` abstraction from `librustdoc`, take 2 💣 In rust-lang/rust#136656 I found out that the for_html field in the Buffer struct was never read, and pondered if Buffer had any utility at all. `@GuillaumeGomez` said he agrees that it can be just removed. So this PR is me removing it. So, r? `@aDotInTheVoid` , maybe? Supersedes #136748
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
So while, err... in the throes of trying to put myself out (😁), I realized that the
for_htmlis never actually read, just initialized.I'm not sure there's much purpose to
Bufferwithout it, might be worth completely yanking it out? It's quite a bit of boilerplate for, basically, allowing us to avoid a few useless.unwrap()s.r? @GuillaumeGomez (again, feel free to re-assign)
and also cc @Mark-Simulacrum because git blame shows you originally added the
Bufferabstraction, maybe you know whatfor_htmlwas originally used for?