add macros for generating Default impls#5257
Conversation
There was a problem hiding this comment.
Thank you for putting this together! It looks very workable. I have a handful of requests but all style/maintainability.
Would you be able to add a commit that makes use of this (replacing s!) in a few small places so we make sure it works in situ? Fine to do that in a separate PR, if you prefer, as long as it's coming soon.
| /// Like [`s`], but also generates a `Default` impl for every struct in the block. | ||
| macro_rules! s_with_default { |
There was a problem hiding this comment.
Nit: move this and s_no_extra_traits_with_default to directly below s_no_extra_traits! to keep them together (this file kind of flows from most important to less important / helpers)
There was a problem hiding this comment.
I meant to order the macros:
ss_parens_no_extra_traitss_with_defaults_no_extra_traits_with_defaultimpl_default
So all the ss are together - currently impl_default is in the middle
| s_with_default! { | ||
| pub struct S3 { | ||
| pub a: u32, | ||
| #[custom_default([0; 64])] |
There was a problem hiding this comment.
Could you make this something other than the default like [1; 64]? To make sure the field attribute is working rather than the derive.
There was a problem hiding this comment.
I missed that there isn't currently a test for this, the change is kind of pointless without it 🙂 could you assert the field values in S3 and the other structs/unions in that area?
There was a problem hiding this comment.
Actually I guess S3's pattern pattern is already checked with CustomDefault
|
Reminder, once the PR becomes ready for a review, use |
|
Added a commit converting @rustbot ready |
| /// for `Default`. If it does not exist, `Default::default()` is used instead. In either case, the | ||
| /// field is added to `processed_fields` with `#[custom_default]` stripped if necessary, and | ||
| /// `impl_default` is invoked again with the remaining fields. | ||
| macro_rules! impl_default { |
There was a problem hiding this comment.
Maybe rename this to struct_with_default or similar since it also emits the struct itself
| s_with_default! { | ||
| pub struct S3 { | ||
| pub a: u32, | ||
| #[custom_default([0; 64])] |
There was a problem hiding this comment.
Actually I guess S3's pattern pattern is already checked with CustomDefault
| impl ::core::default::Default for $name { | ||
| // Field doc comments get forwarded to the initializer alongside `#[cfg]` | ||
| // they're harmless there but the lint fires, so silence it. | ||
| #[allow(unused_doc_comments)] | ||
| fn default() -> Self { | ||
| Self { $($processed_field_defaults)* } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm realizing that something like #[cfg(target_arch = "x86_64")] on the whole struct may not work correctly since impl Default won't get the same config. I think we might need to filter the struct attributes and pass any cfg(...) to Default.
That can be done in a followup though, this gets us most of the way.
(Debug for unions has the same issue, guess we just haven't hit it.)
| s_with_default! { | ||
| struct FieldAttrs { | ||
| #[cfg(target_arch = "x86_64")] | ||
| a: u8, | ||
| #[cfg(not(target_arch = "x86_64"))] | ||
| a: u64, | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you add a #[cfg(target_arch = "x86_64")] field that doesn't have a cfg(not(...)) to go with it? Just thinking about how a: Default::default() technically works on all platforms here, but it might not always.
Description
Adds
impl_default!,s_with_default!, ands_no_extra_traits_with_default!as a start for #4975. Unions get no implicitDefaultso a union field would need to supply one via#[custom_default(unsafe { mem::zeroed::<U>() })]. As discussed,custom_defaultneeds to be the first attribute on a field since the macro matches it literally.Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated