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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Remove implicit collect in simplify::where_clauses #92296

Closed
wants to merge 2 commits into from
Closed

rustdoc: Remove implicit collect in simplify::where_clauses #92296

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3241b63
Remove `retain` call
vacuus Dec 26, 2021
23506c4
Reorder `extend` statements
vacuus Dec 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/librustdoc/clean/simplify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ crate fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> Vec<WP> {

// Look for equality predicates on associated types that can be merged into
// general bound predicates
equalities.retain(|&(ref lhs, ref rhs)| {
let equalities = equalities.into_iter().filter(|&(ref lhs, ref rhs)| {
let (self_, trait_did, name) = if let Some(p) = lhs.projection() {
p
} else {
Expand All @@ -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 }));
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right; equalities should be after all parameters, not in between lifetimes and generics.

Copy link
Contributor Author

@vacuus vacuus Dec 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I originally had an alternate approach:

...the extend statement for params could 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.

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 also not quite sure what the purpose of this PR is. Could you elaborate what you mean by "implicit collect"?

Copy link
Contributor Author

@vacuus vacuus Dec 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

clauses.extend(params.into_iter().map(|(k, (bounds, params))| WP::BoundPredicate {
ty: clean::Generic(k),
bounds,
Expand All @@ -83,7 +84,6 @@ crate fn where_clauses(cx: &DocContext<'_>, clauses: Vec<WP>) -> Vec<WP> {
bounds,
bound_params,
}));
clauses.extend(equalities.into_iter().map(|(lhs, rhs)| WP::EqPredicate { lhs, rhs }));
clauses
}

Expand Down