-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
CTFE: simplify ConstValue by not checking for alignment #63079
Conversation
This comment has been minimized.
This comment has been minimized.
What do you think about removing the |
Hm. Not sure how well that code would factor. With #63075, we actually don't need the |
I don't want to remove the field. Miri will keep using it. Const eval doesn't need it though. If you do UB in const eval, we don't promise we catch it. |
We should probably find a way to make miri's |
With #63075 the field is effectively dead in Miri, though. For now. I think. |
Ah, ok. We can revisit once that is resolved then. Can you make sure that it's documented $somewhere in the process, so that we actually do revisit? r=me with travis green |
Agreed. But factoring that through the |
@bors r=oli-obk |
📌 Commit cfb13b0eeb620dee597a99c4100b1464f589b731 has been approved by |
⌛ Testing commit cfb13b0eeb620dee597a99c4100b1464f589b731 with merge 4034b4c3188a41efb8be7f415d0252d232da06b4... |
💔 Test failed - checks-azure |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Rebased. @oli-obk what do you think of checking alignment in validity even when the machine otherwise ignores alignment (as is now implemented here)? |
I think it's very reasonable. We can always weaken validity checks if we need to. Hardening them is not easy to do. @bors r+ |
📌 Commit 0cf4329 has been approved by |
☀️ Test successful - checks-azure |
I hope the test suite actually covers the problematic cases here?
r? @oli-obk
Fixes #61952