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

Move recursion check for zsts back to read site instead of access check site #71751

Merged
merged 3 commits into from
May 4, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 1, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2020
@RalfJung
Copy link
Member

RalfJung commented May 1, 2020

Hm... honestly I am not convinced if that is the right fix long-term. I anyway plan to change check_offset_align to return an Option<&Allocation> or so, as part of cleaning up the Allocation API in preparation for rust-lang/miri#841. There's just so many other things that keep preempting this task.^^

However, there is no reason to do this re-work now, and this adds a testcase to make sure I find a good way to handle that when I do my planned changes. So, fine for me.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 1, 2020

Hm... honestly I am not convinced if that is the right fix long-term.

Neither am I. I did try to fix it in memory.rs first, but that ended up looking very hacky, so I chose to go for a simple fix here and leave future refactorings to... well the future.

I anyway plan to change check_offset_align to return an Option<&Allocation> or so, as part of cleaning up the Allocation API in preparation for rust-lang/miri#841. There's just so many other things that keep preempting this task.^^

However, there is no reason to do this re-work now, and this adds a testcase to make sure I find a good way to handle that when I do my planned changes. So, fine for me.

@@ -0,0 +1,32 @@
// check-pass
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure "check" is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the const prop one occurred during mir optimization of fn new, which will still happen in check mode, and the static one happened during evaluation of the static's initializer, which also happens in check mode.

@RalfJung
Copy link
Member

RalfJung commented May 1, 2020

r=me with that last question resolved.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 1, 2020

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented May 1, 2020

📌 Commit c64c776 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 May 1, 2020
@bors
Copy link
Contributor

bors commented May 4, 2020

⌛ Testing commit c64c776 with merge 6318d24...

@bors
Copy link
Contributor

bors commented May 4, 2020

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing 6318d24 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2020
@bors bors merged commit 6318d24 into rust-lang:master May 4, 2020
@oli-obk oli-obk deleted the const_ice branch March 16, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Internal Compiler Error in CTFE engine Compiler panic during build of dispatch crate
4 participants