georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.49k stars 194 forks source link

Clippy has a new problem with one of our partial_cmp impls #1088

Closed urschrei closed 6 months ago

urschrei commented 9 months ago
error: incorrect implementation of `partial_cmp` on an `Ord` type
  --> geo/src/algorithm/sweep/point.rs:30:1
   |
30 | /  impl<T: GeoNum> PartialOrd for SweepPoint<T> {
31 | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
   | | _____________________________________________________________-
32 | ||         match self.0.x.partial_cmp(&other.0.x) {
33 | ||             Some(Ordering::Equal) => self.0.y.partial_cmp(&other.0.y),
34 | ||             o => o,
35 | ||         }
36 | ||     }
   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
37 | |  }
   | |__^

I'm not sure, but is this a false positive?

lnicola commented 9 months ago

I think it expects partial_cmp to call into cmp, and not the other way around. The unwrap there seems unfortunate anyway.

I wonder if we can use total_cmp to simplify things.

michaelkirk commented 6 months ago

@lnicola - is something like this what you had in mind?

https://github.com/georust/geo/pull/1134