Skip to content

Use NodeId/HirId instead of DefId for local variables.#44316

Merged
bors merged 1 commit intorust-lang:masterfrom
eddyb:no-local-var-def-id
Sep 11, 2017
Merged

Use NodeId/HirId instead of DefId for local variables.#44316
bors merged 1 commit intorust-lang:masterfrom
eddyb:no-local-var-def-id

Conversation

@eddyb
Copy link
Member

@eddyb eddyb commented Sep 4, 2017

@eddyb eddyb force-pushed the no-local-var-def-id branch 2 times, most recently from 5027135 to b175d84 Compare September 5, 2017 06:19
michaelwoerister
michaelwoerister reviewed Sep 5, 2017
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Nice! It's a bit unfortunate that Def::def_id() can't be used as universally anymore but that's OK. Having fewer DefIds is definitely a win.

One question though: Won't we run into trouble with MIR inlining here as it potentially mixes in locals from other crates?

src/librustc_save_analysis/lib.rs Outdated
Copy link
Member

@michaelwoerister michaelwoerister Sep 5, 2017

Choose a reason for hiding this comment

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

OK.. :D

rls_data::Id should arguably be switched to a (DefPathHash, LocalId) anyway at some point.

@eddyb
Copy link
Member Author

eddyb commented Sep 5, 2017

Won't we run into trouble with MIR inlining here as it potentially mixes in locals from other crates?

Where does MIR refer to variable bindings by some kind of ID? If it does that's a bug IMO.

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 5, 2017
@bors
Copy link
Collaborator

bors commented Sep 7, 2017

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

@michaelwoerister
Copy link
Member

michaelwoerister commented Sep 7, 2017

r=me once rebased.

@eddyb eddyb force-pushed the no-local-var-def-id branch 2 times, most recently from 97b32ae to 1492fcf Compare September 7, 2017 09:53
@eddyb
Copy link
Member Author

eddyb commented Sep 7, 2017

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Sep 7, 2017

📌 Commit 1492fcf has been approved by michaelwoerister

@bors
Copy link
Collaborator

bors commented Sep 8, 2017

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

@eddyb eddyb force-pushed the no-local-var-def-id branch from 1492fcf to da0a47a Compare September 8, 2017 19:01
@eddyb
Copy link
Member Author

eddyb commented Sep 8, 2017

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Sep 8, 2017

📌 Commit da0a47a has been approved by michaelwoerister

@RalfJung RalfJung mentioned this pull request Sep 9, 2017
Merged
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2017
@bors
Copy link
Collaborator

bors commented Sep 10, 2017

⌛ Testing commit da0a47a with merge 3d29f0e...

bors added a commit that referenced this pull request Sep 10, 2017
@bors
Auto merge of #44316 - eddyb:no-local-var-def-id, r=michaelwoerister
3d29f0e 
Use NodeId/HirId instead of DefId for local variables.

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Sep 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 3d29f0e to master...

@bors bors merged commit da0a47a into rust-lang:master Sep 11, 2017
This was referenced Sep 11, 2017
Merged
Merged
Closed
@eddyb eddyb deleted the no-local-var-def-id branch September 11, 2017 04:40
@oli-obk oli-obk mentioned this pull request Sep 12, 2017
Closed
@alexcrichton alexcrichton mentioned this pull request Sep 15, 2017
Closed
@eddyb eddyb mentioned this pull request Jul 14, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 16, 2020
@Dylan-DPC
Rollup merge of rust-lang#74346 - eddyb:reachable-defs, r=nikomatsakis
c531545 
Use LocalDefId instead of HirId for reachable_set elements.

The only `HirId`s being tracked there that don't have matching `DefId`s are local variables, and that's an accident from rust-lang#44316 (where I preserved the old behavior, even if nothing relied on reachability tracking local variables).
tmandry added a commit to tmandry/rust that referenced this pull request Aug 16, 2020
@tmandry
Rollup merge of rust-lang#74346 - eddyb:reachable-defs, r=nikomatsakis
3ef2244 
Use LocalDefId instead of HirId for reachable_set elements.

The only `HirId`s being tracked there that don't have matching `DefId`s are local variables, and that's an accident from rust-lang#44316 (where I preserved the old behavior, even if nothing relied on reachability tracking local variables).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@michaelwoerister michaelwoerister michaelwoerister left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@michaelwoerister michaelwoerister

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eddyb @bors @michaelwoerister @arielb1 @Mark-Simulacrum