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

More Queries for Crate Metadata #41724

Merged
merged 11 commits into from
May 11, 2017
Merged

More Queries for Crate Metadata #41724

merged 11 commits into from
May 11, 2017

Conversation

hackeryarn
Copy link
Contributor

@hackeryarn hackeryarn commented May 3, 2017

This covers a little bit of clean up and the following parts of #41417:

  • fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute>;
  • fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name>;
  • fn trait_of_item(&self, def_id: DefId) -> Option<DefId>;
  • fn impl_parent(&self, impl_def_id: DefId) -> Option<DefId>;
  • fn is_foreign_item(&self, did: DefId) -> bool;
  • fn is_exported_symbol(&self, def_id: DefId) -> bool;

r? @nikomatsakis

@aidanhs
Copy link
Member

aidanhs commented May 3, 2017

Thanks for the PR @achernyak! We'll make sure @nikomatsakis or another reviewer gets to this soon.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 5, 2017

📌 Commit 5a7946d has been approved by nikomatsakis

/// Otherwise, return `None`.
fn trait_of_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Option<DefId> {
if def_id.krate != LOCAL_CRATE {
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Er, wait, this seems surprising. What is going on here exactly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I expected this code to only run when the krate is LOCAL_CRATE, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what this is doing? This returns None if it's not a LOCAL_CRATE.

@@ -119,6 +119,7 @@ provide! { <'tcx> tcx, def_id, cdata
// This is only used by rustdoc anyway, which shouldn't have
// incremental recompilation ever enabled.
fn_arg_names => { cdata.get_fn_arg_names(def_id.index) }
trait_of_item => { cdata.get_trait_of_item(def_id.index) }
Copy link
Contributor

Choose a reason for hiding this comment

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

...and I expected this code to run for other crates.

@nikomatsakis
Copy link
Contributor

@bors r-

@hackeryarn
Copy link
Contributor Author

@nikomatsakis I think I covered what you were asking for.

@hackeryarn
Copy link
Contributor Author

@cramertj This PR should now cover all the simple cases for DefId based queries. I've skipped any queries that would require providing tcx access to functions that currently don't have it, and so on.

/// Otherwise, return `None`.
fn trait_of_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Option<DefId> {
if def_id.krate != LOCAL_CRATE {
return tcx.trait_of_item(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.

I am pretty sure that this if can be removed -- or else there is a bug. After all, tcx.trait_of_item(def_id) should just call back into this code, since this is a query implementation.

@hackeryarn
Copy link
Contributor Author

@nikomatsakis you were absolutely right. It can be removed!

@bors
Copy link
Contributor

bors commented May 9, 2017

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

@hackeryarn hackeryarn force-pushed the master branch 5 times, most recently from 6b3900a to 2486a8e Compare May 9, 2017 17:55
@hackeryarn hackeryarn force-pushed the master branch 3 times, most recently from 41f03e4 to a468875 Compare May 9, 2017 19:40
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit 35812d1 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
More Queries for Crate Metadata

This covers a little bit of clean up and the following parts of rust-lang#41417:
* `fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute>;`
* `fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name>;`
* `fn trait_of_item(&self, def_id: DefId) -> Option<DefId>;`
* `fn impl_parent(&self, impl_def_id: DefId) -> Option<DefId>;`
* ` fn is_foreign_item(&self, did: DefId) -> bool;`
* `fn is_exported_symbol(&self, def_id: DefId) -> bool;`

r? @nikomatsakis
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
More Queries for Crate Metadata

This covers a little bit of clean up and the following parts of rust-lang#41417:
* `fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute>;`
* `fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name>;`
* `fn trait_of_item(&self, def_id: DefId) -> Option<DefId>;`
* `fn impl_parent(&self, impl_def_id: DefId) -> Option<DefId>;`
* ` fn is_foreign_item(&self, did: DefId) -> bool;`
* `fn is_exported_symbol(&self, def_id: DefId) -> bool;`

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

- Successful merges: #41192, #41724, #41873, #41877, #41889
- Failed merges:
@bors bors merged commit 35812d1 into rust-lang:master May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants