Skip to content

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

Merged
bors merged 1 commit into
rust-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 into
rust-lang:masterfrom
eddyb:no-local-var-def-id

Conversation

@eddyb

@eddyb eddyb commented Sep 4, 2017

Copy link
Copy Markdown
Contributor

@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

@michaelwoerister michaelwoerister left a comment

Copy link
Copy Markdown
Member

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?

Comment thread src/librustc_save_analysis/lib.rs Outdated

@michaelwoerister michaelwoerister Sep 5, 2017

Copy link
Copy Markdown
Member

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

eddyb commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

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

bors commented Sep 7, 2017

Copy link
Copy Markdown
Collaborator

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

@michaelwoerister

michaelwoerister commented Sep 7, 2017

Copy link
Copy Markdown
Member

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

eddyb commented Sep 7, 2017

Copy link
Copy Markdown
Contributor Author

@bors r=michaelwoerister

@bors

bors commented Sep 7, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 1492fcf has been approved by michaelwoerister

@bors

bors commented Sep 8, 2017

Copy link
Copy Markdown
Collaborator

☔ 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

eddyb commented Sep 8, 2017

Copy link
Copy Markdown
Contributor Author

@bors r=michaelwoerister

@bors

bors commented Sep 8, 2017

Copy link
Copy Markdown
Collaborator

📌 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

bors commented Sep 10, 2017

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented Sep 11, 2017

Copy link
Copy Markdown
Collaborator

☀️ 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