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

impl From<LocalDefId> for DefId #73076

Closed
wants to merge 1 commit into from
Closed

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 6, 2020

This makes it easier for new contributors (a.k.a me) to convert between types without having to look up https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.LocalDefId.html#method.to_def_id.
I think in some cases it will also cause rustc to suggest .into() when the types don't match which may be helpful even to experienced contributors.

This makes it easier for new contributors (a.k.a me) to convert between
types without having to look up
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.LocalDefId.html#method.to_def_id.
I think in some cases it will also cause rustc to suggest `.into()` when
the types don't match which maybe helpful even to experienced
contributors.
@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2020
@ecstatic-morse
Copy link
Contributor

r? @eddyb since they vetoed this earlier.

FWIW, I think this change is good, but you should also remove to_def_id so as to have one way of doing things.

@rust-highfive rust-highfive assigned eddyb and unassigned estebank Jun 7, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jun 7, 2020

Looks like the old PR was #70956.

@lcnr
Copy link
Contributor

lcnr commented Jun 11, 2020

I personally am also against this change as there are still some locations using DefId where a LocalDefId would be more appropriate and I think it is really helpful to be able to grep for to_def_id.

It might be useful to add a comment to DefId saying "To convert a LocalDefId to a DefId, local_def_id.to_def_id() can be used", so the method can quickly be found be either looking at DefId or LocalDefId.

@jyn514
Copy link
Member Author

jyn514 commented Jun 11, 2020

I personally am also against this change as there are still some locations using DefId where a LocalDefId would be more appropriate and I think it is really helpful to be able to grep for to_def_id.

Ok, you convinced me :) I'll open a new PR adding a comment to DefId.

@jyn514 jyn514 closed this Jun 11, 2020
@jyn514 jyn514 deleted the local-def-into branch June 11, 2020 21:14
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 11, 2020
Add comment about LocalDefId -> DefId

Now there are instructions on how to convert back and forth on both
structs, not just one.

See also rust-lang#73076

r? @lcnr
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.

6 participants