Skip to content

Make Visitor::visit_body take a plain &Body #70449

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 30, 2020
Merged

Make Visitor::visit_body take a plain &Body #70449

merged 2 commits into from
Mar 30, 2020

Conversation

@ecstatic-morse
Copy link
Contributor

ReadOnlyBodyAndCache has replaced &Body in many parts of the code base that don't care about basic block predecessors. This includes the MIR Visitor trait, which I suspect resulted in many unnecessary changes in #64736. This reverts part of that PR to reduce the number of places where we need to pass a ReadOnlyBodyAndCache.

In the long term, we should either give ReadOnlyBodyAndCache more ergonomic name and replace all uses of &mir::Body with it at the cost of carrying an extra pointer everywhere, or use it only in places that actually need access to the predecessor cache. Perhaps there is an even nicer alternative.

r? @Nashenas88

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2020
@ecstatic-morse ecstatic-morse changed the title Make Visitor::visit_body take a simple Body Make Visitor::visit_body take a simple &Body Mar 26, 2020
eddyb
eddyb reviewed Mar 27, 2020
View reviewed changes
src/librustc_mir/borrow_check/borrow_set.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you use &body instead of *body everywhere? I think it would've already coerced except ReadOnlyBodyAndCache is itself a reference internally so it's not passed by reference.

Although, I'm surprised ReadOnlyBodyAndCache exists, wouldn't &BodyAndCache also do the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

ReadOnlyBodyAndCache can only exist if the cache has already been precomputed. That guarantee doesn't hold with &BodyAndCache. That was the only reason a separate type was used. It would have been a lot easier if I could have just used &BodyAndCache.

Copy link
Contributor

@Nashenas88 Nashenas88 Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

See the assert here:

rust/src/librustc/mir/cache.rs

Lines 195 to 198 in a06baa5

assert!(
cache.predecessors.is_some(),
"Cannot construct ReadOnlyBodyAndCache without computed predecessors"
);

Copy link
Contributor

Choose a reason for hiding this comment

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

And the reason a variant with &Body and &mut Cache doesn't work is that there were some places that required such a struct to be Copy, which doesn't hold with the &mut. Though I can't remember which trait was causing that issue.

@eddyb
Copy link
Member

eddyb commented Mar 27, 2020

cc @oli-obk

Nashenas88
Nashenas88 reviewed Mar 27, 2020
View reviewed changes
src/librustc/mir/visit.rs Outdated
Comment on lines +68 to 75
Copy link
Contributor

Choose a reason for hiding this comment

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

So you're having the read-only case not require the cache? I actually hadn't even considered that when I first wrote this. That should actually make this a lot easier to use!

@Nashenas88
Copy link
Contributor

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned Nashenas88 Mar 27, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Mar 29, 2020

r=me after a rebase

@ecstatic-morse ecstatic-morse changed the title Make Visitor::visit_body take a simple &Body Make Visitor::visit_body take a plain &Body Mar 29, 2020
@ecstatic-morse
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Mar 29, 2020

📌 Commit 538cdef has been approved by oli-obk

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2020
@bors
Copy link
Collaborator

bors commented Mar 30, 2020

⌛ Testing commit 538cdef with merge 0afdf43...

@bors
Copy link
Collaborator

bors commented Mar 30, 2020

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 0afdf43 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 30, 2020
@bors bors merged commit 0afdf43 into rust-lang:master Mar 30, 2020
@rust-highfive rust-highfive mentioned this pull request Mar 30, 2020
Closed
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #70449!

Tested on commit 0afdf43.
Direct link to PR: #70449

💔 clippy-driver on windows: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
💔 clippy-driver on linux: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Mar 30, 2020
@rust-highfive
📣 Toolstate changed by rust-lang/rust#70449!
d72f70c 
Tested on commit rust-lang/rust@0afdf43.
Direct link to PR: <rust-lang/rust#70449>

💔 clippy-driver on windows: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
💔 clippy-driver on linux: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
@bors bors mentioned this pull request Mar 30, 2020
Merged
phansch added a commit to phansch/rust-clippy that referenced this pull request Mar 30, 2020
@phansch
Rustup to rust-lang/rust#70449
68ed6cc
@phansch phansch mentioned this pull request Mar 30, 2020
Merged
bors added a commit to rust-lang/rust-clippy that referenced this pull request Mar 30, 2020
@bors
Auto merge of #5390 - phansch:rustupp, r=matthiaskrgr
0254c42 
Rustup to rust-lang/rust#70449

cc rust-lang/rust#70449

changelog: none
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2020
@matthiaskrgr
submodules: update clippy from 70b93aa to e170c84
eef8e48 
Changes:
````
rustup  rust-lang#70536
Rustup to rust-lang#70449
readme: move "how to run single lint" instructions to "Allowing/denying lints" section.
git attribute macros not allowed in submodules
Deprecate REPLACE_CONSTS lint
Bump itertools
````

Fixes rust-lang#70554
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2020
@bors
Auto merge of rust-lang#70574 - matthiaskrgr:submodule_upd, r=Centril
2113659 
submodules: update clippy from 70b93aa to e170c84

Changes:
````
rustup  rust-lang#70536
Rustup to rust-lang#70449
readme: move "how to run single lint" instructions to "Allowing/denying lints" section.
git attribute macros not allowed in submodules
Deprecate REPLACE_CONSTS lint
Bump itertools
````

Fixes rust-lang#70554
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
@matthiaskrgr
submodules: update clippy from 70b93aa to e170c84
d256371 
Changes:
````
rustup  rust-lang/rust#70536
Rustup to rust-lang/rust#70449
readme: move "how to run single lint" instructions to "Allowing/denying lints" section.
git attribute macros not allowed in submodules
Deprecate REPLACE_CONSTS lint
Bump itertools
````

Fixes #70554
@ecstatic-morse ecstatic-morse deleted the visit-body branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@eddyb eddyb eddyb left review comments

@Nashenas88 Nashenas88 Nashenas88 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@oli-obk oli-obk

Labels

merged-by-bors This PR was explicitly merged by bors. 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.

6 participants

@ecstatic-morse @eddyb @Nashenas88 @oli-obk @bors @rust-highfive