-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make <*const/mut T>::offset_from const fn
#63810
Conversation
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
cc @RalfJung this moves the logic of |
This comment has been minimized.
This comment has been minimized.
In terms of const safety this seems fine, since it return an integer that actually is a true integer. It is an unsafe operation, but the cases where it fails during CTFE are all also UB at run-time, but there is no "unconst" weirdness. So this seems reaosonable to me. Well, assuming "all the feature gates". ;) |
Thank you for putting this together! |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ooops, on it. Thanks for the ping |
Resolved review comments |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
LGTM, but what about the CI failure? Also, do you have a Miri branch matching this? I guess the |
@bors r=RalfJung,nikic |
📌 Commit b93f48f has been approved by |
☀️ Test successful - checks-azure |
Use the upstream `exact_div` implementation introduced in rust-lang/rust#63810
And merely 10 weeks later, we finally landed this. :D |
Thanks for pushing this through! |
@mjbshaw so what would be a good next step? We could equip the I'd rather centralize the efforts for this in one place than everyone copy-pasting different variants of the same idea.^^ |
I don't plan on using the |
Given our past experience with lots of incorrect variants of this code spread across half a dozen crates, I think it would be better to share a single known-good implementation that can also be centrally updated. |
In particular, that code still uses |
I generally agree, but I think the macro should live in Computing field offsets is one of the less-risky (comparatively speaking) things my crate does... |
I see. Well seems like I cannot change your mind. ;) I'll Cc you on the PR that adds const support to memoffset; would be good if we could work together on that and synchronize our approaches. |
let a = self.read_immediate(args[0])?.to_scalar()?.to_ptr()?; | ||
let b = self.read_immediate(args[1])?.to_scalar()?.to_ptr()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just realized this is wrong... we need to support integers here as well.
This caused a regression in https://github.com/RalfJung/miri-test-libstd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#66083 fixes that.
Clean up const-hack PRs now that const if / match exist. Closes rust-lang#67627. Cleans up these merged PRs tagged with `const-hack`: - rust-lang#63810 - rust-lang#63786 - rust-lang#61635 - rust-lang#58044 reverting their contents to have the match or if expressions they originally contained. r? @oli-obk There's one more PR in those tagged with `const-hack` that originally wasn't merged (rust-lang#65107). Reading the thread, it looks like it was originally closed because the `const-hack` for the checked arithmetic non-negligibly hurt performance, and because there was no way to manipulate the returned Option at compile time anyway (with neither const if nor const match). Would you like me to add these changes to the changes from this PR here too, now that we have the necessary features?
This reenables offset_of cc @mjbshaw after #63075 broke it