implement clone on builder #2134
Conversation
|
Spoke too soon - I now see that the I suppose an option might be to put implementing Clone on Builder behind a feature gate so that it can be opted-in to when the ability to clone is desired and the need to implement Clone for any ParseCallbacks in use is trvial (or worth the effort). I'll give that a try. |
…ne` (and requires `ParseCallbacks` implementors to implement `Clone` as well)
jrandall
force-pushed
the
2132-implement-clone-on-builder
branch
from
fcdbef4 to
001c6b5
Compare
| @@ -1687,6 +1687,7 @@ impl Drop for Diagnostic { | |||
| } | |||
|
|
|||
| /// A file which has not been saved to disk. | |||
| #[derive(Clone)] | |||
There was a problem hiding this comment.
This is not safe. UnsavedFile keeps pointers to its inner CStrings so at the very least the clone implementation needs to be more subtle.
| @@ -14,7 +14,7 @@ impl ParseCallbacks for EnumVariantRename { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| #[derive(Clone, Debug)] | |||
| } | ||
|
|
||
| #[cfg(feature = "builder-clone")] | ||
| dyn_clone::clone_trait_object!(ParseCallbacks); |
There was a problem hiding this comment.
Can you elaborate on why this is needed instead of just having ParseCallbacks: Clone?
| /// field in Builder when the builder-clone feature is enabled such that | ||
| /// Builder implements Clone. | ||
| #[cfg(feature = "builder-clone")] | ||
| pub trait ParseCallbacks: |
There was a problem hiding this comment.
It is really unfortunate to duplicate all the trait here. Can we avoid this by using a macro instead?
|
I think I'd rather move most of the options to an inner struct and make that clone than this. That said, can't you solve your use-case in #2132 by keeping the custom options in your own data structure or something updating both builders? Or just move the setup to its own function? So something like: fn setup_builder(builder: &mut Builder) {
// All your complex code, call this function twice.
}Or: let mut extra_flags = vec![];
// Compute your flags.
for builder in [&mut with_comments, &mut without_comments] {
builder....
} |
|
Closing as this needs some amount of changes and is not actively being worked on. Feel free to re-open and re-request review though, thanks. |
Implements
CloneonBuilder(and all of the types included within it). Most were straightforward and I simply added derive Clone on the structs, except for the boxedParseCallbackssince it is a trait. I have used thedyn-clonecrate to make this work, provided that the underlying type implementing the trait implements Clone itself.Notably, this does mean that any type used to implement
ParseCallbacksthat does not implementClonewould cause a compilation failure, although I imagine in most cases it should be a simple matter of adding#[derive(Clone)]to the type. Every example I've seen uses a unit struct for theParseCallbacksimplementation, so I guess it would be very easy to fix, but it would nonetheless be a breaking change since code that previously worked would fail to compile after this change until a Clone implementation is added to the type implementingParseCallbacks.The changes made to the parse callbacks tests illustrate the simple fix that would need to be made to client code: 97fed55
Closes #2132