Add VarTuple and VarTupleULE #5511
Conversation
utils/zerovec/src/ule/vartuple.rs
Outdated
| /// See the module for examples. | ||
| #[derive(Debug)] | ||
| #[allow(clippy::exhaustive_structs)] // well-defined type | ||
| pub struct VarTupleULE<A: AsULE, V: ?Sized> { |
There was a problem hiding this comment.
this needs to be repr(packed)
There was a problem hiding this comment.
Oh sure. repr(C, packed), yes?
There was a problem hiding this comment.
It lets me apply repr(C), but repr(packed) and repr(C, packed) say:
error[E0277]: the size for values of type `V` cannot be known at compilation time
--> utils/zerovec/src/ule/vartuple.rs:74:19
|
72 | pub struct VarTupleULE<A: AsULE, V: ?Sized> {
| - this type parameter needs to be `Sized`
73 | pub sized: A::ULE,
74 | pub variable: V,
| ^ doesn't have a size known at compile-time
|
= note: the last field of a packed struct may only have a dynamically sized type if it does not need drop to be run
= help: change the field's type to have a statically known size
There was a problem hiding this comment.
I think repr(C) is fine.
https://doc.rust-lang.org/reference/type-layout.html#the-c-representation
Our safety requirement is that we are align(1) with no padding.
The Rust Reference says:
The alignment of the struct is the alignment of the most-aligned field in it.
Since the two generics are bound to ULE and VarULE which are defined to be align 1, then the repr(C) containing only those fields is defined to be align 1. (Note: I put those bounds on the impl, but I can move them to the struct to satisfy this)
The size and offset of fields is determined by the following algorithm.
Start with a current offset of 0 bytes.
For each field in declaration order in the struct, first determine the size and alignment of the field. If the current offset is not a multiple of the field’s alignment, then add padding bytes to the current offset until it is a multiple of the field’s alignment. The offset for the field is what the current offset is now. Then increase the current offset by the size of the field.
Finally, the size of the struct is the current offset rounded up to the nearest multiple of the struct’s alignment.
Since all field lengths are a multiple of 1, then repr(C) will never add padding bytes.
|
Another way for me to do this without |
utils/zerovec/src/ule/vartuple.rs
Outdated
| // For now we rely on all DST metadata being a usize to extract it via a fake slice pointer | ||
| // Rust doesn't know that `&V` is a fat pointer so we have to use transmute_copy | ||
| debug_assert_eq!(size_of::<*const V>(), size_of::<*const [u8]>()); | ||
| let variable_fat_ptr: *const [u8] = core::mem::transmute_copy(&variable_ptr); |
There was a problem hiding this comment.
nit: this should have a safety comment dedicated to it that starts with Safety:, that describes why the length and the transmute are valid
There was a problem hiding this comment.
also when calling transmute_copy we should redundantly specify the types in a turbofish
utils/zerovec/src/ule/vartuple.rs
Outdated
| let composed_fat_ptr: *const [u8] = core::slice::from_raw_parts(bytes.as_ptr(), metadata); | ||
| debug_assert_eq!(size_of::<*const Self>(), size_of::<*const [u8]>()); | ||
| let composed_ref: *const Self = core::mem::transmute_copy(&composed_fat_ptr); | ||
| &*(composed_ref) |
There was a problem hiding this comment.
nit: we should also have comments explaining what's going on with all the transmutes
|
@Manishearth Ready to merge? |
This abstraction makes things much easier. It is easier to debug than
make_varule, and it composes nicely. I really want to use this for the plurals and patterns things.I had to use
transmute_copybecause the compiler doesn't know that the generic is a fat pointer. But, we know it is, because it is a VarULE type. I don't think there is anything unsound about this PR.