Code cleanup. Make build method generate as const #123
Conversation
Because it's immutable
| @@ -52,7 +52,7 @@ impl ApplyMeta for MutatorAttribute { | |||
| } | |||
|
|
|||
| impl Parse for Mutator { | |||
| fn parse(input: ParseStream) -> syn::Result<Self> { | |||
| fn parse(input: ParseStream<'_>) -> syn::Result<Self> { | |||
There was a problem hiding this comment.
This seems redundant. Is this because of that #![forbid(rust_2018_idioms)] thing?
There was a problem hiding this comment.
Yes. This lets to see at a glance where a borrowing occurs
| @@ -16,7 +18,7 @@ pub struct StructInfo<'a> { | |||
| pub vis: &'a syn::Visibility, | |||
| pub name: &'a syn::Ident, | |||
| pub generics: &'a syn::Generics, | |||
| pub fields: Vec<FieldInfo<'a>>, | |||
| pub fields: Box<[FieldInfo<'a>]>, | |||
There was a problem hiding this comment.
Well, it is not very usefull, but the field is effectively immutable and so there is no need to take away additional 8 bytes for capacity. But the struct is already >2KiB in size so tbh no much difference
| }, | ||
| ) | ||
| }); | ||
| let builder_method_const = Rc::new(OnceCell::new()); |
There was a problem hiding this comment.
Respectfully WTF? Looking at the code, it seems like a simple:
let mut builder_method_const = true;would suffice.
There was a problem hiding this comment.
Borrow checker was not happy about the moved value. It could be done with Rc<Cell<bool>> though.
There was a problem hiding this comment.
I tried it locally and it worked. Had to remove the move from the closure in the map, of course.
I speculate that it didn't work for you because you tried it before adding the collect. The compiler, in a way, was complaining about the very same bug that the collect solved.
Another option is to just do a second iteration:
let builder_method_const = self
.included_fields()
.filter_map(|field| field.builder_attr.via_mutators.as_ref())
.all(|via_mutators| matches!(via_mutators.init, syn::Expr::Lit(_)))
.then(|| quote!(const));But of course - all of this is irrelevant because #123 (comment).
| || quote!(()), | ||
| |via_mutators| { | ||
| let init = &via_mutators.init; | ||
| if !matches!(init, syn::Expr::Lit(_)) { |
There was a problem hiding this comment.
I like to make things explicit. If the build method is to be const, it should be marked as such in the configuration attributes - not implicitly deduced based on some incomplete (literals aren't the only const expressions) heuristics.
There was a problem hiding this comment.
True, makes sense. It was just my compulsive thought to make it const when I saw the macro expansion for my usecase. But now I know that it is not so simple
|
Given how the setter methods are not |
| @@ -70,7 +72,7 @@ impl<'a> StructInfo<'a> { | |||
| }) | |||
| } | |||
|
|
|||
| pub fn builder_creation_impl(&self) -> Result<TokenStream, Error> { | |||
| pub fn builder_creation_impl(&self) -> syn::Result<TokenStream> { | |||
There was a problem hiding this comment.
TBH this is the only change I like in this PR.
Not really. I'm not sure if it would be possible because of deconstruction of |
|
Once rust-lang/rust#67792 gets stabilized, |
#![forbid(rust_2018_idioms)]to thetyped-builder-macrocrate