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

refactor LoweringContext::lower_generics_mut #86298

Closed
wants to merge 1 commit into from
Closed

refactor LoweringContext::lower_generics_mut #86298

wants to merge 1 commit into from

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jun 14, 2021

Reduce nesting by using early exits and factoring out a helper
function. Also add comments explaining why the copying of
?Sized bounds is necessary.

r? @estebank
@rustbot label +A-hir +A-traits +C-cleanup +T-compiler

@rustbot rustbot added A-HIR Area: The high-level intermediate representation (HIR) A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 14, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2021
cjgillot
cjgillot reviewed Jul 20, 2021
View reviewed changes
compiler/rustc_ast_lowering/src/item.rs Outdated
@@ -1379,58 +1379,66 @@ impl<'hir> LoweringContext<'_, 'hir> {
}
}

// Get the `LocalId` of `bound_pred.bounded_ty`, but only if it's a plain type parameter.
fn get_def_id(&mut self, bound_pred: &WhereBoundPredicate) -> Option<LocalDefId> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give a longer name to this function? There are already plenty of ways to get def_ids here.
Perhaps predicate_id?

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 ty_param_id is probably more accurate.

cjgillot
cjgillot reviewed Jul 20, 2021
View reviewed changes
compiler/rustc_ast_lowering/src/item.rs
}
if def_id == self.resolver.local_def_id(param.id) {
add_bounds.entry(param.id).or_default().push(bound.clone());
continue 'next_bound;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop can be refactored into a call to generics.params.iter().find. The matches! test should perhaps be an assertion on the found parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that param.id won't match unless it's a type parameter? I guess duplicate LocalDefIds aren't supposed to happen, and we already know that def_id is for a TyParam. I'm also wary of adding an assertion where there wasn't one in the original code.

On the other hand, the matches! will allow skipping of calls to self.resolver.local_def_id for non-type generic parameters, which could have a performance impact, given that this function is kind of quadratic in the number of generic parameters.

@bors
Copy link
Contributor

bors commented Jul 24, 2021

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

@tlyu
refactor LoweringContext::lower_generics_mut
0771f9c 
Further reduce nesting by using early exits and replacing
the innermost loop with `.find()`. Also eliminates the labeled
branch.

Add comments explaining why the copying of `?Sized` bounds is
necessary.
@tlyu tlyu force-pushed the refactor-lower-generics-mut branch from 30eebcd to 0771f9c Compare July 25, 2021 03:38
@tlyu
Copy link
Contributor Author

tlyu commented Jul 25, 2021

@cjgillot Thanks for the review and for the suggestions! After the merge of #87338, I rebased and did some more deindenting. Using .find() for the innermost loop also allowed the elimination of the labeled break.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
cjgillot
cjgillot reviewed Aug 30, 2021
View reviewed changes
compiler/rustc_ast_lowering/src/item.rs
.map(|d| (d.base_res(), d.unresolved_segments()))
{
Some((Res::Def(DefKind::TyParam, def_id), 0))
if bound_pred.bound_generic_params.is_empty() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

The match expression and this test do not depend on bound. Can those be lifted out of the loop?

@jackh726
Copy link
Member

jackh726 commented Sep 8, 2021

r? @jackh726

This might need a bit of a rebase once #88061 lands, but I think the cleanup here is still useful

@rust-highfive rust-highfive assigned jackh726 and unassigned estebank Sep 8, 2021
@bors
Copy link
Contributor

bors commented Sep 8, 2021

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

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@tlyu Can you please address the merge conflicts

This was referenced Sep 28, 2021
Closed
Merged
@bors bors closed this in 746e465 Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@jackh726 jackh726

Labels
A-HIR Area: The high-level intermediate representation (HIR) A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tlyu @bors @jackh726 @JohnCSimon @cjgillot @estebank @rust-highfive @joelpalmer @inquisitivecrystal @rustbot