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.

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

Changing the structure of mir::interpret::InterpError #62969

Merged
merged 25 commits into from
Aug 2, 2019
Merged

Changing the structure of mir::interpret::InterpError #62969

merged 25 commits into from
Aug 2, 2019

Conversation

saleemjaffer
Copy link
Contributor

@saleemjaffer saleemjaffer commented Jul 25, 2019 •
edited
Loading

Implements this

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Jul 25, 2019
@saleemjaffer
Copy link
Contributor Author

r? @RalfJung @oli-obk

@saleemjaffer saleemjaffer mentioned this pull request Jul 25, 2019
Closed
4 tasks
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@saleemjaffer saleemjaffer force-pushed the declutter_interperror branch 2 times, most recently from 642786d to 8705fc9 Compare July 26, 2019 10:17
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
oli-obk
oli-obk requested changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just some nits then this lgtm.

src/librustc_mir/interpret/cast.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/eval_context.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Jul 31, 2019 •
edited
Loading

@saleemjaffer if you think this is ready for reviews, please remove the "WIP". ;)

RalfJung
RalfJung approved these changes Jul 31, 2019
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

r=me with these nits fixed.

src/librustc/mir/interpret/mod.rs Outdated Show resolved Hide resolved
src/librustc/mir/interpret/mod.rs Show resolved Hide resolved
src/librustc_mir/interpret/eval_context.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
@saleemjaffer saleemjaffer changed the title [WIP] Changing the structure of mir::interpret::InterpError Changing the structure of mir::interpret::InterpError Aug 1, 2019
@RalfJung
Copy link
Member

RalfJung commented Aug 1, 2019

r=me

;-)
This convention means that as far as I am concerned, this PR is good to go with the nits fixed -- so anyone with bors powers could then do at-bors r=RalfJung.

@saleemjaffer
Copy link
Contributor Author

@bors r= RalfJung

@bors
Copy link
Contributor

bors commented Aug 1, 2019

@saleemjaffer: 🔑 Insufficient privileges: Not in reviewers

@saleemjaffer
Copy link
Contributor Author

@RalfJung I do not have bors privileges XD. So I cannot do this.

@RalfJung
Copy link
Member

RalfJung commented Aug 1, 2019

That's okay. :) I use r=me either way, so I don't have to figure out in advance if someone has bors powers.^^

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2019

📌 Commit 00d32e8 has been approved by RalfJung

@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 Aug 1, 2019
@Centril Centril mentioned this pull request Aug 1, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Aug 2, 2019
@Centril
Rollup merge of rust-lang#62969 - saleemjaffer:declutter_interperror,…
51dc78e 
… r=RalfJung

Changing the structure of `mir::interpret::InterpError`

Implements [this](rust-lang/const-eval#4 (comment))
@Centril Centril mentioned this pull request Aug 2, 2019
Merged
bors added a commit that referenced this pull request Aug 2, 2019
@bors
Auto merge of #63214 - Centril:rollup-hdb7dnx, r=Centril
1df512f 
Rollup of 7 pull requests

Successful merges:

 - #62663 (More questionmarks in doctests)
 - #62969 (Changing the structure of `mir::interpret::InterpError`)
 - #63153 (Remove redundant method with const variable resolution)
 - #63189 (Doc improvements)
 - #63198 (Allow trailing comma in macro 2.0 declarations.)
 - #63202 (Fix ICE in #63135)
 - #63203 (Make is_mutable use PlaceRef instead of it's fields)

Failed merges:

r? @ghost
@bors bors merged commit 00d32e8 into rust-lang:master Aug 2, 2019
@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2019 •
edited
Loading

@saleemjaffer yay, it landed. :)

However, as expected, this also broke Miri. Do you want to try and fix that?
You need to clone Miri from https://github.com/rust-lang/miri/ and install the latest (later-than-nightly) rustc with https://github.com/kennytm/rustup-toolchain-install-master. In the Miri src directory:

  • rustup-toolchain-install-master -c rust-src
  • rustup override set <hash> (the hash is shown by the previous step)
  • edit the rust-version file to also just contain that hash
  • run ./miri test; this will fail because the err! macro is gone -- fix those errors, similar to what you did in rustc
  • make a PR :)

@RalfJung RalfJung mentioned this pull request Aug 2, 2019
Merged
@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2019

Never mind, I'll do it because this blocks other work.

Thanks for the PR!

Centril added a commit to Centril/rust that referenced this pull request Aug 3, 2019
@Centril
Rollup merge of rust-lang#63229 - RalfJung:miri-error, r=oli-obk
42dfdc5 
A bit of Miri error cleanup

Some cleanup after rust-lang#62969.

r? @oli-obk
Cc @saleemjaffer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung approved these changes

@tesuji tesuji tesuji left review comments

@oli-obk oli-obk oli-obk approved these changes

Assignees

@RalfJung RalfJung

Labels
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.
7 participants
@saleemjaffer @rust-highfive @bors @RalfJung @oli-obk @tesuji @matthewjasper