rustdoc: Clean up html::format::print_where_clause

[vacuus]

(Arguably) closes #95814

[rust-highfive]

r? @notriddle

(rust-highfive has picked a reviewer for you, use r? to override)

[notriddle]

This doesn't seem any easier-to-follow than the previous version. Here's the things that I really don't like about either version:

1. It uses indent.saturating_sub(1). Does this mean left_padding has the same value when indent=0 as it has when indent=1? If so, isn't that a bug? If not, why not?
2. This is doing string replacement on a value that was already produced with string concatenation above. Isn't this slow? Why isn't it building the string correctly the first time? I would imagine setting up br_with_padding at the beginning of the function and then using it below, instead of doing this clause.replace() stuff, would be faster (though, perhaps, not faster-enough to show up in profiling).
3. clause = left_padding + &clause and clause.insert_str(0, left_padding) are the same thing. One isn't cleaner than the other.

[vacuus]

1. I'm not familiar with rustdoc's internals, so I'm not sure about this.
2. In this function, the only concatenation to clause between the call to comma_sep and the string replacement adds six bytes at most (" "), so the extra overhead seems insignificant. All direct calls to comma_sep are in this file, but tracking down the inputs that are eventually fed into comma_sep seems troublesome.
3. Fair. I think it's marginally more self-evident, but I definitely see where you're coming from. I can revert it and remove the mention of closing the issue if that's best.

[vacuus]

(More of a note to myself)
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=afd5bfce027f84ae4b54cf29f8995fd6
(indent = 3)

f_alternate: false
end_newline: false
clause: "<br>&nbsp;&nbsp; <span class="where">where[COMMA_SEP]</span>"

f_alternate: false
end_newline: true
clause: "&nbsp;&nbsp; <span class="where fmt-newline">where[COMMA_SEP],&nbsp;</span>"

f_alternate: true
end_newline: false
clause: " where[COMMA_SEP]"

f_alternate: true
end_newline: true
clause: " where[COMMA_SEP], "

This doesn't help with the string replacement concern (not sure that can be resolved easily), but this might be useful for simplifying the logic in the bottom half of the function. Even with just two boolean flags, it's quite gnarly.

[notriddle]

Yeah, a match with four branches would be a great way to clean that thing up. The code might be a little longer, but it would be much, much easier to understand.

[vacuus]

I previously implemented it with a match expression. Currently, it's two layers of if-else nesting (one if-else expression in each branch) since I've found that it's slightly more amenable to duplicating code across branches. I think it's still readable.

[Urgau]

Btw, should be rebased after #95813 lands.

[vacuus]

I'm not sure what the comment about adding a space means, so I'm not sure if I'm correct to put in both places where end_newline is true.

[vacuus]

In at least two other functions in this file (clean::PolyTrait::print, clean::Generics::print), I see this kind of thing:
https://github.com/rust-lang/rust/blob/72b5a4981befc6f26ab52b82eac432e159c611ed/src/librustdoc/html/format.rs#L253-L257
, where < > are used if f.alternate() is true. Should that be applied to displaying clean::WherePredicate::BoundPredicate in this function? It currently uses < > in both cases.

I'll preemptively change it; please tell me if I'm wrong.

[GuillaumeGomez]

@vacuus I think you did well to change it. Tests will tell us if we missed something.

[bors]

☔ The latest upstream changes (presumably #96224) made this pull request unmergeable. Please resolve the merge conflicts.

[notriddle]

Sorry about the delay. Rebase it, and r=me.

@bors delegate+

[bors]

✌️ @vacuus can now approve this pull request

[vacuus]

I don't really know about the rollup stuff. I hope it's fine to ignore it. 😅
@bors r=notriddle

[bors]

📌 Commit 5d59c16 has been approved by notriddle

[bors]

⌛ Testing commit 5d59c16 with merge 3d3dafb...

[GuillaumeGomez]

Rollups as used to merge multiple PRs at once. We prevent PRs that have a perf impact or a very big impact on the code to be put in rollups. In this case, it's fine to put it into a rollup though. :)

@bors rollup

[bors]

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing 3d3dafb to master...

[rust-timer]

Finished benchmarking commit (3d3dafb): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.