Add zerofrom::transparent! macro#7788
Conversation
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
In favor of the general implementation. I think the way you handle method docs/pub/etc is clever.
This is exactly along the lines as what I was envisioning for transparent casts for a long time, and is significantly better than my imagined level of complexity.
|
Thanks; if this direction looks good to you then I'll clean up this PR and get it ready for merging. |
| /// Implements [`ZeroFrom`](crate::ZeroFrom) on a transparent type | ||
| /// from a reference to the inner type. | ||
| /// | ||
| /// Also supports creating concrete functions. |
There was a problem hiding this comment.
nit:
| /// Also supports creating concrete functions. | |
| /// Also supports adding concrete conversion functions between various reference types. |
| #[repr(transparent)] | ||
| $(#[$meta:meta])* |
There was a problem hiding this comment.
this is sensitive to attribute ordering, which it shouldn't be
There was a problem hiding this comment.
$(#[$meta:meta])* is greedy and there isn't a clean way to make it non-greedy so I need to put the #[repr(transparent)] at the top.
There was a problem hiding this comment.
A different approach is to have the macro generate the repr but that's less obvious to the reader.
There was a problem hiding this comment.
right. As currently written the macro does not add anything to the struct definition. It just echoes it back out. The only thing is does is add impls which it claims are safe due to the shape of the struct.
| $vis_box:vis fn $fn_box:ident(Box<$type_box:ty>) -> Box<Self>; | ||
| )? | ||
| $( | ||
| @rc |
There was a problem hiding this comment.
let's try to keep custom syntax to a minimum. it should suffice to match on the argument
There was a problem hiding this comment.
I'll try again, but if I match on the argument then every token up to the argument can't be itself a match. macro_rules doesn't do fancy lookahead matching.
There was a problem hiding this comment.
Here's a simplified example if you want to see if you can get it to work:
There was a problem hiding this comment.
Discussion with @robertbastian: if we remove the meta, vis, and function name, then we can match on the argument type. This seems fine.
| #[repr(transparent)] | ||
| /// hello world | ||
| #[derive(Debug)] | ||
| pub(crate) struct Foo([u8; 3]); |
There was a problem hiding this comment.
I don't like type declarations living inside macro invocations. From a call site it's not known that this code is passed through without modifications, the macro could do anything to it (change the repr, add fields, wrap fields, etc).
There was a problem hiding this comment.
So this is why I've overall been very lukewarm on #7607 so far. But I think this is the most maintainable and low-impact version.
We can document what the macro does, and it's pretty easy to verify it, which is why I think this is still a good idea overall.
There was a problem hiding this comment.
I don't love this approach, either. It was not my first or second or third choice at solving the problem. But @Manishearth identified flaws in all the other approaches. My conclusion is that this is the least bad, and better than not solving the problem.
| $( | ||
| $(#[$meta_box])* | ||
| $vis_box fn $fn_box(inner: $crate::internal::Box<$type_box>) -> $crate::internal::Box<Self> { | ||
| unsafe { core::mem::transmute(inner) } |
There was a problem hiding this comment.
Reminder for self: I was lazy and wrote core::mem::transmute here but we should not use transmute on boxes. Box::from_raw is what we should use instead. (#7871)
3 participants
Latest attempt at #7607
Looking for feedback on the general approach, and then on the concrete implementation.
Changelog
zerofrom: Adds macro to derive ZeroFrom and concrete functions for repr(transparent) conversions
zerofrom::transparent!