-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove mir::CastKind::Misc
#102675
Remove mir::CastKind::Misc
#102675
Conversation
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras |
This comment has been minimized.
This comment has been minimized.
CastKind::PointerFromExposedAddress | ||
| CastKind::PointerExposeAddress | ||
| CastKind::Pointer(_) => {} | ||
_ => { |
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.
We should check that the types are as expected here (leaving out CastKind::Pointer(_)
might be fine). This will probably need some deduplication with the typeck code
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.
I am not sure what to check 😅 , with new function mir_cast_kind
we already report a bug with any unexpected cast, and since we built Rvalue
s with that new function is a check really needed?
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.
Yes, because MIR validation doesn't just run after MIR is built, but also after transformations that modify it
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.
I think I modified those too, but it's better to be safe than sorry will add the checks in the next PR.
@@ -42,8 +42,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
let res = self.pointer_from_exposed_address_cast(&src, cast_ty)?; | |||
self.write_immediate(res, dest)?; | |||
} | |||
|
|||
Misc => { | |||
IntToInt | FloatToInt | FloatToFloat | IntToFloat | FnPtrToPtr | PtrToPtr => { | |||
let src = self.read_immediate(src)?; | |||
let res = self.misc_cast(&src, cast_ty)?; |
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.
Please add a FIXME here indicating that we shouldn't have a single misc_cast
for all of them, but handle them separately.
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.
I can do that once this get's merged. 👍
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.
Please add the FIXME now, even if you plan to fix it immediately in a follow-up. :)
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.
But I already did :D
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 good, I misunderstood your comment then. :)
Please squash the commits. Then we can merge and leave the cleanups to follow up PRs |
db0d68d
to
d59c7ff
Compare
@bors r+ |
FloatToInt, | ||
FloatToFloat, | ||
IntToFloat, | ||
PtrToPtr, |
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.
Non-blocking request from the peanut gallery: It's not obvious to me how PtrToPtr
differs from things in Pointer(_)
. Could you please put some doc-comments on the new variants?
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.
"FIXME: Pointer(_)
is legacy code and should be removed. Please submit a PR if you feel inclined." 🙃
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.
I haven't investigated the behavior of Pointer(_)
but old code did it like the below, and some Ptr
to Ptr
casts went through CastKind::Misc
e.g code in src/test/ui/simd/intrinsic/ptr-cast.rs
. However, I could also investigate what Pointer(_)
is actually doing in the next PR.
(_, _) => CastKind::Misc,
Remove `mir::CastKind::Misc` As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast. r? `@oli-obk`
Remove `mir::CastKind::Misc` As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast. r? ``@oli-obk``
Remove `mir::CastKind::Misc` As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast. r? ```@oli-obk```
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#101520 (Allow transmutes between the same types after erasing lifetimes) - rust-lang#102675 (Remove `mir::CastKind::Misc`) - rust-lang#102778 (Fix MIR inlining of asm_unwind) - rust-lang#102785 (Remove `DefId` from some `SelectionCandidate` variants) - rust-lang#102788 (Update rustc-dev-guide) - rust-lang#102789 (Update browser UI test version) - rust-lang#102797 (rustdoc: remove no-op CSS `.rightside { position: initial }`) - rust-lang#102798 (rustdoc: add main-heading and example-wrap link CSS to big selector) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Remove `mir::CastKind::Misc` As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast. r? ````@oli-obk````
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? `@oli-obk`
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? `@oli-obk`
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? ``@oli-obk``
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? ```@oli-obk```
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? ````@oli-obk````
Remove misc_cast and validate types when casting Continuing our work in rust-lang#102675 r? ````@oli-obk````
As discussed in #97649
mir::CastKind::Misc
is not clear, this PR addresses that by creating a new enum variant for every valid cast.r? @oli-obk