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

On-demandify associated item retrieval #40668

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

cramertj
Copy link
Member

Part of #40614.

I also started converting adt_def, but I decided to open a PR with just this bit first to make sure I'm going about this correctly.

r? @nikomatsakis

hir::ItemImpl(.., ref impl_trait_ref, _, ref impl_item_refs) => {
for impl_item_ref in impl_item_refs {
let assoc_item =
tcx.associated_item_from_impl_item_ref(parent_def_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks basically right. I would probably rewrite this for loop (and the one below) to something like:

if let Some(impl_item_ref) = impl_item_refs.iter().find(|i| i.id.node_id == id) {
    return tcx.associated_item_from_impl_item_ref(...); // same parameters you have already
}

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe with an assert! that assoc_item.def_id == def_id at the end =)

@@ -2623,3 +2575,47 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}
}

fn associated_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in rustc::hir, including associated_item_from_{impl,trait}_item_ref.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened up #40697 where we can discuss the precise plan. It feels a bit like this should go into a follow-up PR to me; this PR is just converting to on-demand but keeping the existing structure.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2017

📌 Commit 8e58d9e has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2017
…atsakis

On-demandify associated item retrieval

Part of rust-lang#40614.

I also started converting `adt_def`, but I decided to open a PR with just this bit first to make sure I'm going about this correctly.

r? @nikomatsakis
bors added a commit that referenced this pull request Mar 23, 2017
Rollup of 5 pull requests

- Successful merges: #40612, #40627, #40668, #40715, #40753
- Failed merges:
@bors bors merged commit 8e58d9e into rust-lang:master Mar 23, 2017
@cramertj cramertj deleted the on-demandify-more branch May 27, 2017 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants