Replace unmaintained derivative with derive-where #3402
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryoqun
changed the title
apfitzge
reviewed
Oct 31, 2024
| @@ -569,8 +569,7 @@ mod chained_channel { | |||
|
|
|||
| // P doesn't need to be `: Clone`, yet rustc derive can't handle it. | |||
| // see https://github.com/rust-lang/rust/issues/26925 | |||
| #[derive(Derivative)] | |||
| #[derivative(Clone(bound = "C: Clone"))] | |||
| #[derive_where(Clone)] | |||
There was a problem hiding this comment.
thoughts on just implementing Clone here instead of adding another dependency that may become stale in the future. It seems really straight-forward:
impl<P, C: Clone> Clone for ChainedChannelReceiver<P, C> {
fn clone(&self) -> Self {
Self {
reciever: self.receiver.clone(),
aux_receiver: self.aux_receiver.clone(),
context: self.context.clone(),
}
}
}
There was a problem hiding this comment.
@apfitzge sorry for the delay. took some time to consolidate my thoughts with some research.
in short, I'd like to avoid manual impl and use this new crate (the one called derive-where, among others with omitted reasons after long evaluation.) for following personal preferences:
- i'm generally inclined to using crates as much as possible because I'm open-source maxi and upstreaming-pr happy guy :)
- i'm moderately against boilerplate code, which is error-prone, imo. yes, to recite, I'm saying this, even if we're talking about straight-forward manual impl of just 9 LoC. (less code == less maintenance; in fact, upcoming pr would have to tweak the manual impl.)
- oftentimes, crate shopping (shh, my hobby) could refresh one's rust knowledge. so, i hope this habit could be rewarded in some way. for example, i noticed
ImmutableDeserializedPacketcan be simplified. - in this particular case, switching cost is very small, should the new one could be stale (unlike say,
bincode, which would break our wire-format should we switch). - i think (slightly) longer build time by adding another dep can generally be offset by increased productivity or concise code base in this case.
- thanks to rust opt, all tested clone impls (std, manual, those crates mentioned on this pr) generates the same machine code.
There was a problem hiding this comment.
😄 ImmutableDeserializedPacket is awful and is going to go away entirely, which is the simplest it can become.
apfitzge
approved these changes
Nov 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
the crate
derivativeseems to be unmaintained: mcarton/rust-derivative#117Summary of Changes
Alternatives considered:
https://crates.io/crates/derive_more(EDIT: turned out not to be usable)fyi, after this pr, the
derivativeis still used by other transitive deps (i.e. included inCargo.lock.). Also,educeandderive_moreare also used by yet other transtive deps.