Remove interior mutability in mir predecessors cache #64736
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
oli-obk
reviewed
Sep 24, 2019
oli-obk
reviewed
Sep 24, 2019
oli-obk
reviewed
Sep 24, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Nashenas88
commented
Sep 25, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
reviewed
Sep 25, 2019
Nashenas88
commented
Sep 28, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Nashenas88
force-pushed
the
mir_predecessors_cache_cleanup
branch
from
64be10d to
ef33d75
Compare
Nashenas88
commented
Sep 28, 2019
Nashenas88
commented
Sep 28, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #64886) made this pull request unmergeable. Please resolve the merge conflicts. |
Nashenas88
commented
Oct 3, 2019
oli-obk
reviewed
Oct 3, 2019
Nashenas88
force-pushed
the
mir_predecessors_cache_cleanup
branch
from
c52d128 to
176fcc9
Compare
oli-obk
reviewed
Oct 4, 2019
oli-obk
reviewed
Oct 4, 2019
oli-obk
reviewed
Oct 4, 2019
Nashenas88
force-pushed
the
mir_predecessors_cache_cleanup
branch
from
e549f6f to
3eaad56
Compare
|
@bors r=oli-obk |
|
📌 Commit 3eaad56 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Dec 2, 2019
…li-obk Remove interior mutability in mir predecessors cache
|
☀️ Test successful - checks-azure |
This was referenced Dec 2, 2019
bjorn3
reviewed
Dec 2, 2019
| } | ||
|
|
||
| impl BodyCache<'tcx> { | ||
| pub fn ensure_predecessors(&mut self) { |
There was a problem hiding this comment.
This doesn't need to take self by mut ref.
There was a problem hiding this comment.
It does. Cache's ensure_predecessors takes self by mut ref (https://github.com/rust-lang/rust/pull/64736/files/3eaad564d25402013bdf5591c453c916b49cdf93#diff-f1f20e32c0d3522f2f33135df0f4696bR47), and that requires that self.cache here below is taken by mut ref. That's only possible if you have &mut self. Or is there some detail I'm overlooking?
flip1995
added a commit
to flip1995/rust-clippy
that referenced
this pull request
Dec 2, 2019
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Dec 2, 2019
Rustup to rust-lang/rust#64736 cc rust-lang/rust#64736 Fixes #4872 changelog: none
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Dec 2, 2019
Changes: ```` Normalize custom ICE test Rustup to rust-lang#64736 Use assert_crate_local for a more explicit error Rustup to rust-lang#66789 account for external macro in MISSING_INLINE_IN_PUBLIC_ITEMS lint build(tests/fmt): use shared target dir chore: fix and split some ui tests on 32bit system build: set up build job for i686 targets remove needless my_lint ui test git quiet deploy: cd to out/ before adding files to git Less needless_doctest_main false positives fmt Feed the dog Use rustc_env instead of exec_env for test Make triggering this lint less likely 📎 Use exec_env to set backtrace level and normalize output Update custom ICE function with latest rustc Use Clippy version in ICE message Add custom ICE message that points to Clippy repo Fix master deployment Run update_lints Add projections check to EUV for escape analysis Use infer_ctxt Move use_self to nursery Use `println!` on success instead of `eprintln!` Revert "Disable chalk integration test. Output too large" Remove the old integration-tests.sh script Use rust implementation for integration tests in CI Rust implementation of integration test Don't error on clippy.toml of dependencies Fix categorizations Fix arguments on ExprUseVisitor::new euv moved from middle to typeck cmt_ -> Place build: check if RTIM is not installed make use of Result::map_or trigger string_lit_as_bytes when literal has escapes Remove negative float literal checks. Enable deny-warnings feature everywhere in CI Remove unused debugging feature implemented `as_conversions` lint fixing a typo [comparison_chain] rust-lang#4827 Check `core::cmp::Ord` is implemented add a good example for the approx_const lint Add suggested good cases in docs for lifetimes lint ````
eddyb
reviewed
Dec 3, 2019
| result[tgt].push(bb); | ||
| } | ||
| #[derive(Clone, Debug, HashStable, RustcEncodable, RustcDecodable, TypeFoldable)] | ||
| pub struct BodyCache<'tcx> { |
There was a problem hiding this comment.
This should probably be named BodyAndCache to alleviate confusion.
eddyb
reviewed
Dec 3, 2019
| } | ||
| } | ||
|
|
||
| impl Index<BasicBlock> for ReadOnlyBodyCache<'a, 'tcx> { |
There was a problem hiding this comment.
I don't think you need this, since you have Deref.
eddyb
reviewed
Dec 3, 2019
|
|
||
|
|
||
| impl Deref for ReadOnlyBodyCache<'a, 'tcx> { | ||
| type Target = Body<'tcx>; |
There was a problem hiding this comment.
Oof, this is a common mistake for wrappers of references.
The target should be the reference, &'a Body<'tcx>, so that you can actually get e.g. &'tcx mir::BasicBlockData<'tcx> when 'a = 'tcx.
I wonder if this PR added any lifetimes to existing code to work around that (likely not codegen because that was already wrongly using &'a Body<'tcx> instead of &'tcx Body<'tcx>).
EDIT: thankfully doesn't seem like it. I will likely fix the Deref impl in #65947.
eddyb
reviewed
Dec 3, 2019
| fn create_funclets<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
| mir: &'a Body<'tcx>, | ||
| fn create_funclets<'a, 'b, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
| mir: &'b Body<'tcx>, |
eddyb
added a commit
to eddyb/rust
that referenced
this pull request
Dec 3, 2019
Merged
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 30, 2020
Make `Visitor::visit_body` take a plain `&Body` `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 rust-lang#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
flip1995
pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
May 5, 2020
Changes: ```` Normalize custom ICE test Rustup to rust-lang/rust#64736 Use assert_crate_local for a more explicit error Rustup to rust-lang/rust#66789 account for external macro in MISSING_INLINE_IN_PUBLIC_ITEMS lint build(tests/fmt): use shared target dir chore: fix and split some ui tests on 32bit system build: set up build job for i686 targets remove needless my_lint ui test git quiet deploy: cd to out/ before adding files to git Less needless_doctest_main false positives fmt Feed the dog Use rustc_env instead of exec_env for test Make triggering this lint less likely 📎 Use exec_env to set backtrace level and normalize output Update custom ICE function with latest rustc Use Clippy version in ICE message Add custom ICE message that points to Clippy repo Fix master deployment Run update_lints Add projections check to EUV for escape analysis Use infer_ctxt Move use_self to nursery Use `println!` on success instead of `eprintln!` Revert "Disable chalk integration test. Output too large" Remove the old integration-tests.sh script Use rust implementation for integration tests in CI Rust implementation of integration test Don't error on clippy.toml of dependencies Fix categorizations Fix arguments on ExprUseVisitor::new euv moved from middle to typeck cmt_ -> Place build: check if RTIM is not installed make use of Result::map_or trigger string_lit_as_bytes when literal has escapes Remove negative float literal checks. Enable deny-warnings feature everywhere in CI Remove unused debugging feature implemented `as_conversions` lint fixing a typo [comparison_chain] rust-lang#4827 Check `core::cmp::Ord` is implemented add a good example for the approx_const lint Add suggested good cases in docs for lifetimes lint ````
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.