Initial commit of DstLayout::for_dst #541
Conversation
jswrenn
force-pushed
the
dst-layout-from-dst
branch
2 times, most recently
from
824e41f
to
85ed994
Compare
jswrenn
force-pushed
the
dst-layout-from-dst
branch
3 times, most recently
from
d0fcb15
to
21cea7e
Compare
jswrenn
force-pushed
the
dst-layout-from-dst
branch
4 times, most recently
from
5927d5b
to
8956931
Compare
| // SAFETY: TODO | ||
| SizeInfo::Sized { _size } |
There was a problem hiding this comment.
I'm not convinced this is sound for non-repr(C) types. Rust reserves the right to add excess padding.
There was a problem hiding this comment.
Oh yeah very good point. Maybe rename to for_repr_c_dst or something? And add a safety precondition?
There was a problem hiding this comment.
What should the UX of the derive be? Some possibilities:
- unconditionally require
repr(C)on structs. - don't unconditionally require
repr(C), but expand toDstLayout::for_type::<Self>()on non-repr(C)types, andDstLayout::for_repr_c_dstonrepr(C)types.
There was a problem hiding this comment.
Generally I'd prefer the latter, but how can we make the UX of the error message good when the user tries to derive on an unsized, non-repr(C) type?
There was a problem hiding this comment.
Done, but not sure yet how good the UX can be. This still needs UI tests.
jswrenn
force-pushed
the
dst-layout-from-dst
branch
3 times, most recently
from
05fd194
to
da522c6
Compare
src/lib.rs
Outdated
| @@ -3908,6 +3965,80 @@ mod tests { | |||
| } | |||
| } | |||
|
|
|||
| mod for_dst { | |||
There was a problem hiding this comment.
What's the thinking behind having these in a nested module as opposed to just naming them test_dst_layout_for_sized etc?
There was a problem hiding this comment.
- reduces repetition in the names
- makes it possible to code-collapse the entire group in an editor
My preference (not in this PR) would be even to go a little further and drop test_ from all tests. All of the tests are already located in a module called "tests".
| panic!(concat!( | ||
| "cannot compute layout of `", | ||
| stringify!($ty), | ||
| "`; type is larger than zerocopy supports" |
There was a problem hiding this comment.
Leaving a TODO so we don't forget: We should have an issue link here so there's a way for users to report that they need support for larger types.
jswrenn
force-pushed
the
dst-layout-from-dst
branch
5 times, most recently
from
ef0f250
to
4a2c8f0
Compare
This will be used by the custom derive of `KnownLayout` to compute the `DstLayout` for unsized types. Makes progress on #29 Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
jswrenn
force-pushed
the
dst-layout-from-dst
branch
from
4a2c8f0
to
558ca65
Compare
This will be used by the custom derive of
KnownLayoutto compute theDstLayoutfor unsized types.Makes progress on #29