Skip to content

Add a Place::is_indirect method to determine whether a Place contains a Deref projection - #64005

Merged
bors merged 2 commits into
rust-lang:masterfrom
ecstatic-morse:is-indirect
Sep 5, 2019
Merged

Add a Place::is_indirect method to determine whether a Place contains a Deref projection#64005
bors merged 2 commits into
rust-lang:masterfrom
ecstatic-morse:is-indirect

Conversation

@ecstatic-morse

@ecstatic-morseecstatic-morse commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

Working on #63860 requires tracking some property about each local. This requires differentiating Places like x and x.field[index] from ones like *x and *x.field, since the first two will always access the same region of memory as x while the latter two may access any region of memory. This functionality is duplicated in various places across the compiler. This PR adds a helper method to Place which determines whether that Place has a Deref projection at any point and changes some existing code to use the new method.

I've not converted qualify_consts.rs to use the new method, since it's not a trivial conversion and it will get replaced anyway by #63860. There may be other potential uses besides the two I change in this PR.

r? @oli-obk

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2019
This returns whether a `Place` references the same region of memory
as its base, or equivalently whether it contains a `Deref` projection.
This is helpful for analyses that must track state for locals, since an
assignment to `x` or `x.field` is fundamentally different than one to
`*x`, which may mutate any memory region.
@ecstatic-morse
ecstatic-morseforce-pushed the is-indirect branch 2 times, most recently from bd8c5d1 to e9e075fCompareAugust 29, 2019 20:56

@oli-obkoli-obk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the changes in this PR it looks to me like all use cases could use the find_local function instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about changing the true to !place.is_indirect()?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That works better.

@oli-obk

Copy link
Copy Markdown
Contributor

r? @spastorino this looks like something right up your alley

Comment threadsrc/librustc_mir/dataflow/impls/borrowed_locals.rs Outdated
@spastorino

Copy link
Copy Markdown
Member

Nice work and looks good to me. I'm not sure I like the name of the function but can't think of a better one right now. Any other ideas?, @oli-obk do you consider the fn name good enough? any better ideas?.

@oli-obk

Copy link
Copy Markdown
Contributor

@bors r+

Nope, no better ideas here

@bors

bors commented Sep 4, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 96ac02b has been approved by oli-obk

@borsbors 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 Sep 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
Add a `Place::is_indirect` method to determine whether a `Place` contains a `Deref` projection
Working on rust-lang#63860 requires tracking some property about each local. This requires differentiating `Place`s like `x` and `x.field[index]` from ones like `*x` and `*x.field`, since the first two will always access the same region of memory as `x` while the latter two may access any region of memory. This functionality is duplicated in various places across the compiler. This PR adds a helper method to `Place` which determines whether that `Place` has a `Deref` projection at any point and changes some existing code to use the new method.
I've not converted `qualify_consts.rs` to use the new method, since it's not a trivial conversion and it will get replaced anyway by rust-lang#63860. There may be other potential uses besides the two I change in this PR.
r? @oli-obk
@CentrilCentril mentioned this pull request Sep 5, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
Add a `Place::is_indirect` method to determine whether a `Place` contains a `Deref` projection
Working on rust-lang#63860 requires tracking some property about each local. This requires differentiating `Place`s like `x` and `x.field[index]` from ones like `*x` and `*x.field`, since the first two will always access the same region of memory as `x` while the latter two may access any region of memory. This functionality is duplicated in various places across the compiler. This PR adds a helper method to `Place` which determines whether that `Place` has a `Deref` projection at any point and changes some existing code to use the new method.
I've not converted `qualify_consts.rs` to use the new method, since it's not a trivial conversion and it will get replaced anyway by rust-lang#63860. There may be other potential uses besides the two I change in this PR.
r? @oli-obk
@CentrilCentril mentioned this pull request Sep 5, 2019
bors added a commit that referenced this pull request Sep 5, 2019
Rollup of 15 pull requests
Successful merges:
- #62860 (Stabilize checked_duration_since for 1.38.0)
- #63549 (Rev::rposition counts from the wrong end)
- #63985 (Stabilize pin_into_inner in 1.39.0)
- #64005 (Add a `Place::is_indirect` method to determine whether a `Place` contains a `Deref` projection)
- #64031 (Harden `param_attrs` test wrt. usage of a proc macro `#[attr]`)
- #64038 (Check impl trait substs when checking for recursive types)
- #64043 (Add some more tests for underscore imports)
- #64092 (Update xLTO compatibility table in rustc book.)
- #64110 (Refer to "`self` type" instead of "receiver type")
- #64120 (Move path parsing earlier)
- #64123 (Added warning around code with reference to uninit bytes)
- #64128 (unused_parens: account for or-patterns and `&(mut x)`)
- #64141 (Minimize uses of `LocalInternedString`)
- #64142 (Fix doc links in `std::cmp` module)
- #64148 (fix a few typos in comments)
Failed merges:
r? @ghost
@bors
bors merged commit 96ac02b into rust-lang:masterSep 5, 2019
@ecstatic-morse
ecstatic-morse deleted the is-indirect branch October 6, 2020 01:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: 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.

5 participants

@ecstatic-morse@oli-obk@spastorino@bors@rust-highfive