-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#[builder(default)] now honors struct impls #62
Conversation
Cool, thanks again you for your initiative. :-) Here are some thoughts and wishes for this PR:
Let me know if you need any help/advice. |
one addition: If it's not asking too much I would prefer if the generated code was something like Foo {
foo: 42,
..::std::default::Default::default()
} instead of let __default = ..::std::default::Default::default();
Foo {
foo: 42,
bar: __default.bar,
} PS: It's what people would actually write and has no risk of clashing with other identifiers. |
I actually tried to do that, but I couldn't figure out a way for the macro at compile time to figure out which fields would be populated at runtime. I'm almost done with the other changes; will have a new version up soon.
I think I understand what you're saying here; once the latest revision passes muster then I'm happy to look at refactoring that.
I added a changelog entry; which docs did you want updated? The |
The deprecation warning is in the correct function - I've found the issue and updated the PR. At the moment, this has the side-effect of emitting the warning multiple times; I haven't figured out how to keep it down to 1 yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin-kiegel I broke the tests and I'm not sure how.
Removed unnecessary clone() by having StructDeprecationNotes take a slice rather than own a Vec of the messages
@TedDriggs The skeptic tests have some issues with cargo cache. Since I activated both cargo cache and skeptic tests in master, I'm sure this failing build is just spurious nonsense and unrelated to this PR. The failing test looks at least similar to this bug in skeptic. I'll look into a workaround - which might involve disabling skeptic tests and/or cargo cache again. :-) I'll also take a look at your PR this weekend. |
…yped fields Made default="..." refer to a function which is invoked to build the default struct, rather than propagating down to each field.
(match *expr { | ||
DefaultExpression::Struct | ||
| DefaultExpression::Trait => "::std::default::Default::default()".parse(), | ||
DefaultExpression::Explicit(ref body) => format!("{}()", body).parse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this for closer scrutiny, as it touches on the issue described in #65 (this change is just at the struct level; the field level can be a separate PR).
@@ -65,6 +68,11 @@ impl<'a> ToTokens for BuildMethod<'a> { | |||
BuilderPattern::Immutable => quote!(&self), | |||
}; | |||
let doc_comment = &self.doc_comment; | |||
let default = if let Some(ref expr) = self.struct_default { | |||
quote!(let __default : #target_ty = #expr;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible optimization: Check if all fields are explicitly initialized and avoid allocating the extra object if it won't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I can see to do this is to generate an and-block calling is_some()
on every field in the builder, and allowing __default
to be Option<#target_ty>
instead of #target_ty
. I'd then have to call unwrap
for every implicitly-instantiated field; I don't know if there's a good way to determine which path is more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a way to generate the code like this:
Foo {
foo: 42,
..#struct_default // <-- only emitted if struct_default is set.
}
This requires to propagate the options a bit differently. I will commit this as soon as I checked everything and added some more tests. :-)
@TedDriggs I've started some minor refactoring on top of your branch (before your last two commits) - is it ok, if I rebase + push my changes to your branch, too? Alternatively I could just push to a different branch and then maybe let you merge my changes into your PR - especially if you want to continue in parallel. However I'm not yet done. What do you think? |
@TedDriggs Btw. did you delete 2 comments? In my inbox I counted 5 inline-comments, but I count only three in your review - or am I missing something? |
Hm, looks like something went wrong during rebase. I'll rebase again and probably force push. |
Ok, it turns out I actually overlooked something. My current approach does not work, if there is a struct default expression, but no field default expression. It looks like a temporary value is indeed unavoidable - I guess I will change it back, but not force push. :-) CurrentFoo {
foo: match self.foo {
Some(value) => value,
None => /* ... _no_ field default, but we have to return something .... */,
},
../* struct default expression */
} Previouslet __default = /* struct default expression */;
Foo {
foo: match self.foo {
Some(value) => value,
None => __default.foo,
},
} |
Addresses issue #61.