Skip to content
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.

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

rename Memory::get methods to get_raw to indicate their unchecked nature #66043

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 2, 2019

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the interp module as far as Miri is concerned -- with the exception of the uninit intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Nov 2, 2019
@cramertj
Copy link
Member

cramertj commented Nov 4, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2019

📌 Commit 0aee0dd has been approved by cramertj

@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 Nov 4, 2019
@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

does priroda need these methods?

Yes, it needs get to print an allocation: https://github.com/oli-obk/priroda/blob/c6a4126b2bf721716bf5336dd0d848f6780d4d53/src/render/locals.rs#L350

@RalfJung
Copy link
Member Author

RalfJung commented Nov 4, 2019

@bjorn3 okay, so we should likely still allow immutable access. But we can shut down mutable access eventually.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

I have actually been thinking about allowing the user to mutate state (including allocations) instead of just observing it. However I havent worked on priroda anymore for 5 months according to github.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2019

Maybe renaming the functions to _get_raw and _get_raw_mut will discourage usage even more. This is already used for _intern_substs and several other interners which get wrapped by the function without _ as prefix.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 4, 2019

For now I don't think things are bad enough that we need to resort to leading underscores...

Centril added a commit to Centril/rust that referenced this pull request Nov 5, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
@Centril
Copy link
Contributor

Centril commented Nov 6, 2019

Toolstate failure in #66138 (comment), @bors rollup=never p=-1

Please revert ^-- in a bit.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

A Miri failure shouldn't be a problem even during the beta week...?

@Centril
Copy link
Contributor

Centril commented Nov 6, 2019

🤷‍♂ -- beats me; it seems to have caused a failure tho... but I think toolstate blockage is finally over.

@bors p=0 rollup=maybe

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

Ah, clippy-driver also got broken in that rollup. Whether that was this PR or another one I don't know. I guess we'll see.^^

@RalfJung
Copy link
Member Author

RalfJung commented Nov 6, 2019

Looks like clippy was affected by another PR in the rollup: #66150

@bors
Copy link
Contributor

bors commented Nov 7, 2019

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Nov 8, 2019

Rebased.

@bors r=cramertj

@bors
Copy link
Contributor

bors commented Nov 8, 2019

📌 Commit 15ec8f7 has been approved by cramertj

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2019
@bors
Copy link
Contributor

bors commented Nov 8, 2019

⌛ Testing commit 15ec8f7 with merge b21093f8fcb2591907a739fd7a05cd8500727521...

Centril added a commit to Centril/rust that referenced this pull request Nov 8, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
@Centril
Copy link
Contributor

Centril commented Nov 8, 2019

@bors retry rolled up.

Centril added a commit to Centril/rust that referenced this pull request Nov 8, 2019
rename Memory::get methods to get_raw to indicate their unchecked nature

Some recent Miri PRs started using these methods when they should not; this should discourage their use.

In fact we could make these methods private to the `interp` module as far as Miri is concerned -- with the exception of the `uninit` intrinsic which will hopefully go away soon. @bjorn3 @oli-obk does priroda need these methods? It would be great to be able to seal them away.
bors added a commit that referenced this pull request Nov 8, 2019
Rollup of 5 pull requests

Successful merges:

 - #65785 (Transition future compat lints to {ERROR, DENY} - Take 2)
 - #66007 (Remove "here" from "expected one of X here")
 - #66043 (rename Memory::get methods to get_raw to indicate their unchecked nature)
 - #66154 (miri: Rename to_{u,i}size to to_machine_{u,i}size)
 - #66188 (`MethodSig` -> `FnSig` & Use it in `ItemKind::Fn`)

Failed merges:

r? @ghost
@bors bors merged commit 15ec8f7 into rust-lang:master Nov 8, 2019
@RalfJung RalfJung deleted the memory-get-raw branch November 9, 2019 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

6 participants