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

Tidy up some uses of csearch::get_item_path #27643

Merged
merged 2 commits into from
Aug 16, 2015
Merged

Conversation

mitaa
Copy link
Contributor

@mitaa mitaa commented Aug 10, 2015

(this incidentally fixes an error message where the paths separator is " " instead of "::")

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

pub fn get_item_name(tcx: &ty::ctxt, def: ast::DefId) -> ast::Name {
let cstore = &tcx.sess.cstore;
let cdata = cstore.get_crate_data(def.krate);
decoder::get_item_name(&*cstore.intr, &*cdata, def)
Copy link
Member

Choose a reason for hiding this comment

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

do you need the *s here?

@nrc
Copy link
Member

nrc commented Aug 10, 2015

lgtm, r+ with the nits addressed

@mitaa
Copy link
Contributor Author

mitaa commented Aug 10, 2015

Nits addressed.

@arielb1
Copy link
Contributor

arielb1 commented Aug 11, 2015

@mitaa

I would prefer to place get_item_name in middle::ty rather than csearch (including the same-crate case) - csearch isn't supposed to be accessed by random code.

@mitaa
Copy link
Contributor Author

mitaa commented Aug 11, 2015

@arielb1
Ahh, I see. (updated)

I think this should be fine now.

@mitaa
Copy link
Contributor Author

mitaa commented Aug 11, 2015

(csearch::get_item_name still exists, but it is only called from the new method item_name on ctxt.
I've kept it because it currently seems to be a convention that the decoder module is only accessed through other metadata modules like csearch.)

@arielb1
Copy link
Contributor

arielb1 commented Aug 11, 2015

Could you trim the is_local checks around the places you changed? r+ modulo that.

@mitaa
Copy link
Contributor Author

mitaa commented Aug 12, 2015

Updated.

r? @arielb1

@rust-highfive rust-highfive assigned arielb1 and unassigned nrc Aug 12, 2015
_ => false,
}
}
intrinsic && self.tcx.item_name(def_id) == "transmute"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should check that this is indeed a foreign fn, but you can't really define extern "rust-intrinsic" fn statics anyway.

@arielb1
Copy link
Contributor

arielb1 commented Aug 16, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2015

📌 Commit d81feb8 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Aug 16, 2015

⌛ Testing commit d81feb8 with merge a49d9ba...

bors added a commit that referenced this pull request Aug 16, 2015
(this incidentally fixes an error message where the paths separator is " " instead of "::")
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.

5 participants