Skip to content

remove visit_terminator_kind from MIR visitor #72814

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Jun 19, 2020

Conversation

RalfJung
Copy link
Member

For some reason, we had both visit_terminator and visit_terminator_kind. In contrast, for Statement we just have visit_statement. So this cleans things up by removing visit_terminator_kind and porting its users to visit_terminator.

@rust-highfive
Copy link
Contributor

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 May 31, 2020
@RalfJung RalfJung force-pushed the mir-visir-terminator branch from 6de019c to fef7702 Compare May 31, 2020 10:16
@@ -84,6 +88,8 @@ impl<'visit, 'cx, 'tcx> Visitor<'tcx> for GatherUsedMutsVisitor<'visit, 'cx, 'tc
);
self.remove_never_initialized_mut_locals(*into);
}

// FIXME: no super_statement?
Copy link
Member Author

@RalfJung RalfJung May 31, 2020

Choose a reason for hiding this comment

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

I didn't change anything here, but it seems rather suspicious that this does not call super_statement -- in particular since there can be uses of locals inside statements, and this visitor does use visit_local.

Cc @spastorino @matthewjasper

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a bug

Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewjasper I added this now, at the end of this function. I was not sure if beginning or end was appropriate though.

Copy link
Member

@spastorino spastorino Jun 10, 2020

Choose a reason for hiding this comment

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

If this is a bug, wouldn't it make sense to add a test to cover this case?.

Copy link
Member Author

@RalfJung RalfJung Jun 10, 2020

Choose a reason for hiding this comment

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

I have no idea what this code is doing or where it is invoked. I can do this as a drive-by fix, but I can't spend further research time on this to figure out enough to be able to write a testcase.

If someone wants to take over and do that, that would be great. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic in this area is a bit of a mess and is somewhat redundant. The actual test is being able to do some of that clean up without breaking the existing tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that seems to work.^^

@RalfJung RalfJung force-pushed the mir-visir-terminator branch from 5e40fa7 to 76e5fd5 Compare June 10, 2020 07:41
@spastorino
Copy link
Member

About the missing super calls, I'm not sure if those were accidentally left out or if those methods are intended as a let's overwrite this method. To be honest I'm not sure cc @oli-obk

@bors

This comment has been minimized.

@RalfJung RalfJung force-pushed the mir-visir-terminator branch from 4f01cbb to 704a063 Compare June 12, 2020 07:50
@RalfJung
Copy link
Member Author

r? @matthewjasper

@bors

This comment has been minimized.

@RalfJung RalfJung force-pushed the mir-visir-terminator branch from 704a063 to 827ccf7 Compare June 16, 2020 09:25
@RalfJung
Copy link
Member Author

Rebased. @matthewjasper @eddyb given the frequency of conflicts, would be nice to get this reviewed soon-ish. :) Cc @oli-obk maybe they can help.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 16, 2020

I have alredy reviewed this a few days ago. Just looked over the changes after the rebase, lgtm

@bors r+

r? @oli-obk

@bors
Copy link
Collaborator

bors commented Jun 16, 2020

📌 Commit 827ccf7 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 Jun 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 16, 2020
…i-obk

remove visit_terminator_kind from MIR visitor

For some reason, we had both `visit_terminator` and `visit_terminator_kind`. In contrast, for `Statement` we just have `visit_statement`. So this cleans things up by removing `visit_terminator_kind` and porting its users to `visit_terminator`.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
…i-obk

remove visit_terminator_kind from MIR visitor

For some reason, we had both `visit_terminator` and `visit_terminator_kind`. In contrast, for `Statement` we just have `visit_statement`. So this cleans things up by removing `visit_terminator_kind` and porting its users to `visit_terminator`.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
…i-obk

remove visit_terminator_kind from MIR visitor

For some reason, we had both `visit_terminator` and `visit_terminator_kind`. In contrast, for `Statement` we just have `visit_statement`. So this cleans things up by removing `visit_terminator_kind` and porting its users to `visit_terminator`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
…arth

Rollup of 17 pull requests

Successful merges:

 - rust-lang#70551 (Make all uses of ty::Error delay a span bug)
 - rust-lang#71338 (Expand "recursive opaque type" diagnostic)
 - rust-lang#71976 (Improve diagnostics for `let x += 1`)
 - rust-lang#72279 (add raw_ref macros)
 - rust-lang#72628 (Add tests for 'impl Default for [T; N]')
 - rust-lang#72804 (Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position)
 - rust-lang#72814 (remove visit_terminator_kind from MIR visitor)
 - rust-lang#72836 (Complete the std::time documentation to warn about the inconsistencies between OS)
 - rust-lang#72968 (Only highlight doc search results via mouseover if mouse has moved)
 - rust-lang#73034 (Export `#[inline]` fns with extern indicators)
 - rust-lang#73315 (Clean up some weird command strings)
 - rust-lang#73320 (Make new type param suggestion more targetted)
 - rust-lang#73361 (Tweak "non-primitive cast" error)
 - rust-lang#73425 (Mention functions pointers in the documentation)
 - rust-lang#73428 (Fix typo in librustc_ast docs)
 - rust-lang#73447 (Improve document for `Result::as_deref(_mut)` methods)
 - rust-lang#73476 (Added tooltip for should_panic code examples)

Failed merges:

r? @ghost
@bors bors merged commit e0b59b2 into rust-lang:master Jun 19, 2020
@RalfJung RalfJung deleted the mir-visir-terminator branch June 20, 2020 10:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants