Skip to content
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: more impl fmt::Display #138455

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yotamofek
Copy link
Contributor

Continuation of #137425 and #136828 and #136784
Working towards getting rid of the write_str helper
r? @GuillaumeGomez (if you want!)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Mar 13, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@yotamofek
Copy link
Contributor Author

yotamofek commented Mar 13, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged. Otherwise, you can ignore this comment.

This is intentional, I'm pulling in latest version of either with impl fmt::Write for Either (cc & thanks @jswrenn and @cuviper)

@yotamofek
Copy link
Contributor Author

Perf is neutral locally, but I'll request a run on CI just to be sure.

&added_classes,
let s = format!(
"\n{}",
highlight::render_example_with_highlighting(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this change is good considering we need to allocate a string in any case but relying on format!.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will end up behaving exactly the same. It will construct the string piece-by-piece.
It would only matter if we used String::with_capacity, otherwise .pushing into a string directly, or using format!(), are the same in terms of reallocations and such.
(maybe I'm missing something?)

@@ -6,16 +6,17 @@
//! Use the `render_with_highlighting` to highlight some rust code.
Copy link
Member

@GuillaumeGomez GuillaumeGomez Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in this file: I don't expect any improvement in performance considering it's always pushing into a string and it makes the code more complex.

Copy link
Contributor Author

@yotamofek yotamofek Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this PR in itself isn't performance.

My end-goal is to try to make ALL the rendering lazy, and then we could ideally write into a BufWriter that wraps the actual file, instead of writing everything to a String first, and then to the file system.
I do believe that that would be good for performance, because it should minimize the number of reallocs needed for rendering a single file. And maybe we could reuse the underlying buffer from BufWriter, which would be even better.
But such a change would be huge and I don't want to do it in one PR, so in the meanwhile I'm trying to put out small PRs that make progress towards that goal without hurting performance - so this PR being neutral in a perf run would be a success, as far as I'm concerned.

I also want to get rid of that write_str helper I introduced in #136784, since it's very verbose (because it needs format_args!()) and I intended it as a stopgap measure.

The only change to the code in this file (if you view the diff w/o whitespace) is the fmt::from_fn() and extra indentation it causes, which isn't great, but I think that the removal of all the format_args!() makes up for it. That's just my personal opinion, of course :)

Anyways, if you don't like the direction I'm going in let me know, and, obviously, feel free to close this PR if you think it's not improving the code at all! I'm thankful for the time you put into reviewing all my PRs 🙏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the rest of the changes. In highlight.rs though a bit less.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I undid the unrelated changes in highlight.rs, let me know if it's better now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but still the same, for now I only see it as a code complexity increase for no gain. ^^'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, was traveling 😁
Undid the changes in this file, will figure it out in the future.

@yotamofek yotamofek force-pushed the pr/rustdoc/more-impl-display branch from f8f7bc2 to f22e94c Compare March 14, 2025 14:54
@yotamofek yotamofek force-pushed the pr/rustdoc/more-impl-display branch from f22e94c to cf9cf6b Compare March 25, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants