Clippy Fixes #2282
Clippy Fixes #2282
Conversation
| impl PartialOrd for Estimate { | ||
| #[allow(clippy::non_canonical_partial_ord_impl)] | ||
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | ||
| self.bandwidth.partial_cmp(&other.bandwidth) | ||
| Some(self.cmp(other)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the Ord implementation:
impl Ord for Estimate {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.bandwidth.cmp(&other.bandwidth)
}
}
There was a problem hiding this comment.
Seems correct. loss is ignored consistently for eq and cmp.
| // Priority ordering is complex, disable Clippy warning. | ||
| #[allow(clippy::non_canonical_partial_ord_impl)] | ||
| fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { | ||
| Some(self.cmp(other)) |
There was a problem hiding this comment.
Touching this always makes me a little nervous but we have test coverage and that seems to have passed
There was a problem hiding this comment.
It seems like the before and after do the same, even if the semantics of the comparison are surprising.
| // Ignore priority if ID matches. | ||
| if self.id == other.id { | ||
| return Some(cmp::Ordering::Equal); | ||
| return cmp::Ordering::Equal; |
There was a problem hiding this comment.
I'm worried about correctness due to this line.
Ignoring the other fields if the ids are equal seems very very bad as it doesn't allow for correct sorting for a collection of elements when there are 2 with same id but differences in other fields.
There was a problem hiding this comment.
Its kinda implicit in the overall design but where StreamPriorityKey would be used, there cannot be two objects with the same stream ID (If there were, we'd have bigger issues 😂 )
| fn cmp(&self, other: &Self) -> cmp::Ordering { | ||
| // `partial_cmp()` never returns `None`, so this should be safe. | ||
| self.partial_cmp(other).unwrap() | ||
| cmp::Ordering::Greater |
There was a problem hiding this comment.
I don't know what the requirements for cmp are and what are the consequences of having elements that return different answers if you swap the argument order.
I think this is a case where the id could be compared.
In the partial order case, this case may want to return None.
There was a problem hiding this comment.
That would break the prioritization algorithm which needs these types of streams to be round-robined. This is a bit of a trick, newly added incremental streams of the same urgency to "go to the back", so by removing then adding a stream when it's been visited (e.g. to send data) we can ensure the next visit hits the next stream of this type.
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Seems like an improvement. Thanks for the lint fixes.
3 participants
Clippy fixes for Rust 1.92
https://rust-lang.github.io/rust-clippy/stable/index.html#/non_canonical_partial_ord_impl