rustdoc: Remove implicit collect in simplify::where_clauses
#92296
Conversation
rustbot
added
the
T-rustdoc
|
r? @CraftSpider (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Admittedly, I'm not good at judging lifetimes. Would rearranging the |
|
Moving |
|
Looking into the code deeper, it looks like this may change the order of bounds in the output. No test appears to capture this, I'm not sure if this is considered acceptable breakage. I approve the code changes otherwise though, so I'll delegate to someone who's more familiar with the frontend. r? GuillaumeGomez |
|
Whoops |
|
@CraftSpider raised a very good point. I recently worked on this in #89098. I didn't add a test then for whatever reason so I'll add so we can be sure it's not breaking anything. |
|
@GuillaumeGomez the reason you didn't add the test is because of #89098 (comment). Now that we have HTML snapshot tests, just add this line (you can assign me on the PR): // @snapshot where_clause - '//*[@id="implementors-list"]//span[@class="where fmt-newline"]' |
| @@ -73,6 +73,7 @@ crate fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> Vec<WP> { | |||
| clauses.extend( | |||
| lifetimes.into_iter().map(|(lt, bounds)| WP::RegionPredicate { lifetime: lt, bounds }), | |||
| ); | |||
| clauses.extend(equalities.map(|(lhs, rhs)| WP::EqPredicate { lhs, rhs })); | |||
There was a problem hiding this comment.
This doesn't seem right; equalities should be after all parameters, not in between lifetimes and generics.
There was a problem hiding this comment.
I originally had an alternate approach:
...the extend statement for
paramscould use.iter()instead and copy/clone where necessary
but I just realized that params is borrowed mutably in the retain (filter, in this PR) call, so it wouldn't work. It seems like I'll have to close this PR.
There was a problem hiding this comment.
I'm also not quite sure what the purpose of this PR is. Could you elaborate what you mean by "implicit collect"?
There was a problem hiding this comment.
I was under the impression that the retain call iterates through the collection, filters, and implicitly collects the elements that are supposed to remain. (Though I remembered seeing #91497, so retain seemed even less desirable even if it didn't implicitly call collect.) In any case, I had hoped to avoid anything beyond the necessary costs of iterating and filtering.
There was a problem hiding this comment.
My understanding of retain is that it just moves memory around; there shouldn't be any implicit collects. However, this rustdoc code does iterate twice.
This code could probably be cleaned up in a way that would improve the performance, but it'd likely be a larger change than just this. So I'm going to close this PR for now. Please feel free to re-open it (or open a new PR) if you find a way to get it working.
|
Closing due to #92296 (comment). |
No description provided.