offset_of: allow (unstably) taking the offset of slice tail fields #126150
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
| let lang_item = self.tcx.require_lang_item(LangItem::Sized, None); | ||
| self.require_type_meets(ty, span, code, lang_item); |
There was a problem hiding this comment.
require_type_is_sized should be a tiny bit more concise
There was a problem hiding this comment.
That also duplicates some of the logic, though.
| for ty in tys.iter().take(index + 1) { | ||
| self.require_type_is_sized(ty, expr.span, ObligationCauseCode::Misc); | ||
| } |
There was a problem hiding this comment.
What's the justification for removing this? Trivial that this is implied by the well-formedness of the type?
For annoying level of completetness, is there a test that exercises this? If not, pls add. Maybe something like offset_of!(([i32], i32), 1)
There was a problem hiding this comment.
I made sure all tests still pass and couldn't see any reason why the loop existed, and there was no comment explaining it, so I removed it.
We don't have a similar loop for ADTs either. Not sure what is the point of a test for an obviously ill-formed type... and why only for tuples? Such a test, if we want it, is entirely orthogonal for this PR and could cover all sorts of axes of ill-definedness of a type.
EDIT: Turns out we had a good place to put such a test. 🤷
| @@ -3391,8 +3391,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| { | |||
There was a problem hiding this comment.
Can't leave an inline comment bc github is dumb, but there's a:
// FIXME: DSTs with static alignment should be allowed
...above for the enum case.
But we don't allow that for enums, since their tails are required to be sized. Can you tweak that message so it doesn't look like something that someone may be interested in fixing, since it's not really actionable unless we were to tweak enums in general.
|
I think this is a good improvement on its own, but as a sidenote: I wish there would be a trait for types with static alignment, so that this is not just a special case. |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a595f32): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 3.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
Fields of type
[T]have a statically known offset, so there is no reason to forbid them inoffset_of!. This PR adds theoffset_of_slicefeature to allow them.I created a tracking issue: #126151.