Introduce total_cmp on GeoNum to avoid unwraps
#1134
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
In particular the latest `home` crate used by bindgen, required by proj, needs rust-1.70+ Also updates CI containers to latest
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
2 times, most recently
from
5d5ff20 to
ac35b65
Compare
|
Is the TotalOrd trait requirement a result of the MSRV bump? |
|
Ah, just read the commit message. It's due to a lint introduced in Rust 1.75 |
urschrei
approved these changes
Jan 2, 2024
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
from
ac35b65 to
ab1103b
Compare
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
from
ab1103b to
f73379c
Compare
michaelkirk
commented
Jan 3, 2024
| @@ -241,7 +241,7 @@ fn vwp_wrapper<T, const INITIAL_MIN: usize, const MIN_POINTS: usize>( | |||
| epsilon: &T, | |||
| ) -> Vec<Vec<Coord<T>>> | |||
| where | |||
| T: CoordFloat + RTreeNum + HasKernel, | |||
| T: GeoFloat + RTreeNum, | |||
There was a problem hiding this comment.
Without this change I get:
error[E0599]: no method named `intersects` found for struct `CachedEnvelope<geo_types::Line<T>>` in the current scope
This error was happening because the previous constraint, CoordFloat + HasKernel was enough to hit the impl GeoNum for T blanket impl. Since GeoFloat is basically CoordFloat + HasKernel it makes sense to just use GeoFloat to fix this and similar future problems if we add other traits.
This fixes a new lint introduced in rust 1.75 Fixes #1088
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
from
f73379c to
014ed5c
Compare
lnicola
reviewed
Jan 3, 2024
lnicola
reviewed
Jan 3, 2024
lnicola
approved these changes
Jan 3, 2024
The previous approach worked, but then there was feedback that the trait should maybe be "more complete" and implemented on additional types. Really, I introduced the trait to solve a problem: we have some undesirable unwraps in the codebase with some of our floating type geometries. I didn't care to actually introduce a general-purpose TotalOrd trait that was useful outside of that context.
There's so little you could do with HasKernel that isn't also a GeoNum. I'm not sure there's value in having two traits.
michaelkirk
changed the title
total_cmp on GeoNum to avoid unwraps
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
from
6b9ad93 to
9ea617b
Compare
michaelkirk
force-pushed
the
mkirk/rust-msrv-1.70
branch
from
9ea617b to
dfdd025
Compare
frewsxcv
approved these changes
Jan 10, 2024
|
@rmanoka - did you want to weigh in on this? I've gotten a few positive responses so far so I'm going to go ahead and merge it in a couple days if I don't hear from you. |
rmanoka
approved these changes
Jan 10, 2024
|
Looks good to me too! +1 for merge. |
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.
CHANGES.mdif knowledge of this change could be valuable to users.In particular the latest
homecrate used by bindgen, required by proj, needs rust-1.70+Also updates CI containers to latest