-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Change tag_field
to FieldIdx
in Variants::Multiple
#142005
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
Conversation
It was already available as a generic parameter anyway, and it's not like we'll ever put a tag in the 5-billionth field.
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_codegen_ssa |
oh sure. |
@@ -26,7 +26,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |||
// No need to validate that the discriminant here because the | |||
// `TyAndLayout::for_variant()` call earlier already checks the | |||
// variant is valid. | |||
let tag_dest = self.project_field(dest, tag_field)?; | |||
let tag_dest = self.project_field(dest, tag_field.as_usize())?; |
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.
project_field
should probably take FieldIdx
... but that can be a future PR.
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.
IIRC this hits the problem that a bunch of these things are also used for arrays, and I've never dug in enough to know whether it's actually ok to convert a bunch of these things over to FieldIdx
-- if this is used for [u8; 1 << 32]
then the capped-at-0xFFFFFF00
FieldIdx
wouldn't work.
(Though of course usize
already doesn't work today either for cross-compiling to a 64-bit target from a 32-bit host, but I suspect that nobody actually ever does that so nobody's cared about it being broken.)
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.
project_field
should not be used for arrays, we have project_index
for that. (Those used to be one function but I refactored that a long time ago.)
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.
Oh, wait, InterpCx. I was thinking of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.FieldsShape.html#method.offset and such.
If you say it'll work always here, I can give that a shot (in another PR).
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.
If you say it'll work always here
I think it should, but I can't be sure without just trying it myself.^^
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 was thinking of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.FieldsShape.html#method.offset and such.
Yeah that should be cleaned up to separate field offsets from array element offsets.
Rollup of 11 pull requests Successful merges: - #141890 (Add link to correct documentation in htmldocck.py) - #141932 (Fix for async drop inside async gen fn) - #141960 (Use non-2015 edition paths in tests that do not test for their resolution) - #141968 (Run wfcheck in one big loop instead of per module) - #141969 (Triagebot: Remove `assign.users_on_vacation`) - #141985 (Ensure query keys are printed with reduced queries) - #141999 (Visit the ident in `PreciseCapturingNonLifetimeArg`.) - #142005 (Change `tag_field` to `FieldIdx` in `Variants::Multiple`) - #142017 (Fix incorrect use of "recommend" over "recommended") - #142024 (Don't refer to 'this tail expression' in expansion.) - #142025 (Don't refer to 'local binding' in extern macro.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #142005 - scottmcm:fieldidx-in-variantsmultiple, r=workingjubilee Change `tag_field` to `FieldIdx` in `Variants::Multiple` It was already available as a generic parameter anyway, and it's not like we'll ever put a tag in the 5-billionth field. This is a first part of pulling smaller pieces out of #138759, so r? workingjubilee
…-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang#142005 (comment)
Rollup merge of #142103 - scottmcm:fieldidx-in-interp, r=oli-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in #142005 (comment)
Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang/rust#142005 (comment)
Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang/rust#142005 (comment)
It was already available as a generic parameter anyway, and it's not like we'll ever put a tag in the 5-billionth field.
This is a first part of pulling smaller pieces out of #138759, so
r? workingjubilee